Skip to content

[KYUUBI #7109] Ignore the ? in backticks #7125

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

Closed
wants to merge 1 commit into from

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Jul 2, 2025

Why are the changes needed?

We will split the sql by ? when we use KyuubiPreparedStatement. But there exist corner case when ? exist in backticks.
For example, below sql contains ?, but we shouldn't split it by ?.

SELECT `(ds|hr)?+.+` FROM sales

More details can find at https://hive.apache.org/docs/latest/languagemanual-select_27362043/#regex-column-specification

Hive upstream fix - HIVE-29060

How was this patch tested?

UT.

Was this patch authored or co-authored using generative AI tooling?

NO.

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2025

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (e98ad7b) to head (7140980).

Files with missing lines Patch % Lines
...c/main/java/org/apache/kyuubi/jdbc/hive/Utils.java 0.00% 4 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #7125   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         700     700           
  Lines       43374   43377    +3     
  Branches     5873    5875    +2     
======================================
- Misses      43374   43377    +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pan3793
Copy link
Member

pan3793 commented Jul 2, 2025

Kyuubi JDBC Driver is derived from Hive JDBC Driver, was this fixed in Hive project?

@ruanwenjun
Copy link
Member Author

Kyuubi JDBC Driver is derived from Hive JDBC Driver, was this fixed in Hive project?

Hive doesn't have a fix for this. 😢

@pan3793
Copy link
Member

pan3793 commented Jul 2, 2025

What about spark-sql? The ported JDBC driver code does not have sufficient test coverage, we should be careful when touching this code

@ruanwenjun
Copy link
Member Author

What about spark-sql? The ported JDBC driver code does not have sufficient test coverage, we should be careful when touching this code

spark-sql supports this if we set spark.sql.parser.quotedRegexColumnNames=true. I agree with you — it's dangerous and might introduce bugs.

Actually, my initial idea was to get it merged into the community first, and then cherry-pick it into our version. Going through the community review process can help reduce the likelihood of bugs.

@pan3793
Copy link
Member

pan3793 commented Jul 2, 2025

let me take a look at how spark-sql processes this.

@ruanwenjun
Copy link
Member Author

ruanwenjun commented Jul 4, 2025

@ruanwenjun
Copy link
Member Author

ruanwenjun commented Jul 7, 2025

Hive has fixed this yesterday apache/hive@9a68ebc @pan3793

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix7109 branch from 48c1037 to 7140980 Compare July 7, 2025 06:49
@pan3793 pan3793 added this to the v1.10.3 milestone Jul 7, 2025
@pan3793 pan3793 closed this in 4e40f94 Jul 7, 2025
pan3793 pushed a commit that referenced this pull request Jul 7, 2025
### Why are the changes needed?
We will split the sql by `?` when we use `KyuubiPreparedStatement`. But there exist corner case when ? exist in backticks.
For example, below sql contains `?`, but we shouldn't split it by `?`.
```sql
SELECT `(ds|hr)?+.+` FROM sales
```
More details can find at https://hive.apache.org/docs/latest/languagemanual-select_27362043/#regex-column-specification

Hive upstream fix - HIVE-29060

### How was this patch tested?

UT.

### Was this patch authored or co-authored using generative AI tooling?

NO.

Closes #7125 from ruanwenjun/dev_wenjun_fix7109.

Closes #7109

7140980 [ruanwenjun] [KYUUBI #7109] Ignore the ? in backticks

Lead-authored-by: Wenjun Ruan <wenjun@apache.org>
Co-authored-by: ruanwenjun <zyb@wenjuns-MacBook-Pro-2.local>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 4e40f94)
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793
Copy link
Member

pan3793 commented Jul 7, 2025

Thanks, merged to master/1.10

@ruanwenjun ruanwenjun deleted the dev_wenjun_fix7109 branch July 8, 2025 01:15
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.

3 participants