Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring kyuubi.engine.type in short name #5773

Closed
wants to merge 1 commit into from

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Nov 24, 2023

🔍 Description

Issue References 🔗

Currently, the value candidates of kyuubi.engine.type are SPARK_SQL, FLINK_SQL, TRINO, HIVE_SQL, JDBC, CHAT, which are inconsistent.

Describe Your Solution 🔧

We may want to unify them by removing the tailing _SQL in the future, before doing that, let's support such values as aliases first.

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

configuring kyuubi.engine.type as SPARK or FLINK or HIVE is invalid, must be SPARK_SQL or FLINK_SQL or HIVE_SQL.

Behavior With This Pull Request 🎉

all of SPARK, SPARK_SQL, FLINK, FLINK_SQL, HIVE, HIVE_SQL is valid

Related Unit Tests


Checklists

📝 Author Self Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

  • Pull request title is okay.
  • No license issues.
  • Milestone correctly set?
  • Test coverage is ok
  • Assignees are selected.
  • Minimum number of approvals
  • No changes are requested

Be nice. Be informative.

@codecov-commenter
Copy link

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e22a7f7) 61.36% compared to head (27bb75b) 61.37%.
Report is 1 commits behind head on master.

Files Patch % Lines
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 40.00% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #5773   +/-   ##
=========================================
  Coverage     61.36%   61.37%           
  Complexity       23       23           
=========================================
  Files           607      607           
  Lines         35917    35922    +5     
  Branches       4927     4930    +3     
=========================================
+ Hits          22040    22046    +6     
+ Misses        11494    11489    -5     
- Partials       2383     2387    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pan3793 pan3793 added this to the v1.9.0 milestone Nov 29, 2023
Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to unify them by removing the tailing _SQL in the future, before doing that, let's support such values as aliases first.

I don't quite follow necessity and priority assumptions here.

@pan3793
Copy link
Member Author

pan3793 commented Nov 30, 2023

We may want to unify them by removing the tailing _SQL in the future, before doing that, let's support such values as aliases first.

I don't quite follow necessity and priority assumptions here.

  1. current options are inconsistent, half with _SQL suffix and half without
  2. I was asked frequently does SPARK_SQL support runs Scala/Python statements.
  3. I was also asked why Hive is called HIVE_SQL but Trino is called TRINO without _SQL
  4. I encountered users struggling for a quite long time and eventually found "Oops, I set it to FLINK, but it should be FLINK_SQL"

Copy link

Thanks for the PR! This PR is being closed due to inactivity. This isn't a judgement on the merit of the PR in any way. If this is still an issue with the latest version of Kyuubi, please reopen it and ask a committer to remove the Stale tag!

Thank you for using Kyuubi!

@github-actions github-actions bot added the Stale label Mar 10, 2024
@github-actions github-actions bot closed this Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants