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.

Sign up for GitHub

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

Jump to bottom

Support to return null value for OperationsResource rowset #4372

Closed
wants to merge 3 commits into from
Closed

Support to return null value for OperationsResource rowset #4372

wants to merge 3 commits into from

Conversation

turboFei
Copy link
Member

@turboFei turboFei commented Feb 20, 2023 •
edited
Loading

Why are the changes needed?

With restful api, for query select cast(null as int) c1

Before

the result is 0

After

the result is null.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@turboFei
Copy link
Member Author

cc @pan3793

@turboFei turboFei self-assigned this Feb 20, 2023
@turboFei turboFei added this to the v1.7.0 milestone Feb 20, 2023
@turboFei turboFei requested a review from pan3793 February 20, 2023 06:00
@turboFei
Copy link
Member Author

cc @simon824

bowenliang123
bowenliang123 approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM. Good catch.

And can we tract a general method getFieldValueOrNullfor this ?

@bowenliang123 bowenliang123 reopened this Feb 20, 2023
@turboFei
Copy link
Member Author

And can we tract a general method getFieldValueOrNullfor this ?

@bowenliang123 I thought about that, but it only be used for one time.

@turboFei turboFei added the kind:bug This is a clearly a bug label Feb 20, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #4372 (b15f1eb) into master (5b2c406) will increase coverage by 0.00%.
The diff coverage is 71.42%.

@@            Coverage Diff            @@
##             master    #4372   +/-   ##
=========================================
  Coverage     53.70%   53.70%           
  Complexity       13       13           
=========================================
  Files           562      563    +1     
  Lines         30834    30877   +43     
  Branches       4157     4164    +7     
=========================================
+ Hits          16558    16584   +26     
- Misses        12728    12740   +12     
- Partials       1548     1553    +5
Impacted Files Coverage Δ
...ache/kyuubi/server/api/v1/OperationsResource.scala 72.44% <71.42%> (+4.59%) ⬆️
...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala 59.09% <0.00%> (-7.58%) ⬇️
...in/java/org/apache/kyuubi/client/BatchRestApi.java 97.56% <0.00%> (-2.44%) ⬇️
...mon/src/main/scala/org/apache/kyuubi/Logging.scala 41.25% <0.00%> (-1.25%) ⬇️
...che/kyuubi/server/KyuubiTHttpFrontendService.scala 60.00% <0.00%> (-0.69%) ⬇️
...g/apache/kyuubi/operation/BatchJobSubmission.scala 75.27% <0.00%> (-0.55%) ⬇️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 97.45% <0.00%> (-0.07%) ⬇️
.../org/apache/kyuubi/jdbc/hive/KyuubiConnection.java 5.75% <0.00%> (-0.01%) ⬇️
.../org/apache/kyuubi/config/KyuubiReservedKeys.scala 0.00% <0.00%> (ø)
...va/org/apache/kyuubi/client/util/VersionUtils.java 63.63% <0.00%> (ø)
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@turboFei
Copy link
Member Author

thanks, merging to master and branch-1.7

@turboFei turboFei closed this in afc1d5e Feb 20, 2023
turboFei added a commit that referenced this pull request Feb 20, 2023
@turboFei
[KYUUBI #4372] Support to return null value for OperationsResource ro…
c4955e6 
…wset

### _Why are the changes needed?_

With restful api, for query `select cast(null as int) c1`

#### Before
the result is `0`

#### After
the result is `null`.
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4372 from turboFei/null_value.

Closes #4372

b15f1eb [fwang12] nit
5f16855 [fwang12] fix
45c60dd [fwang12] check is set

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: fwang12 <fwang12@ebay.com>
(cherry picked from commit afc1d5e)
Signed-off-by: fwang12 <fwang12@ebay.com>
@turboFei turboFei deleted the null_value branch February 20, 2023 08:22
SusurHe pushed a commit to SusurHe/incubator-kyuubi that referenced this pull request Feb 21, 2023
Merge branch 'master' of github.com:apache/kyuubi
4cd3d23 
* 'master' of github.com:apache/kyuubi: (1557 commits)
  [KYUUBI apache#3951][FOLLOWUP] Audit the rest request params
  [KYUUBI apache#4377] Grant execute permission to release scripts
  [KYUUBI apache#4374] Release uploading should include kyuubi-spark-connector-hive
  [KYUUBI apache#4267] Show warning if SessionHandle is invalid
  [KYUUBI apache#4385] [DOCS] Refine release process
  [KYUUBI apache#4352] Support System.gc() with periodic GC interval
  [KYUUBI apache#4152][FOLLOWUP] LDAP configurations should be server-only
  [KYUUBI apache#4373] Using SVN_STAGING_REPO instead of SVN_STAGING_REPO in the release script to fix echo message
  [KYUUBI apache#4372] Support to return null value for OperationsResource rowset
  [KYUUBI apache#4371] Fix typo in `kyuubi_ecosystem.drawio`
  [KYUUBI apache#4216] Support to transfer client version for kyuubi hive jdbc and rest client sdk
  [KYUUBI apache#4345] Add the doc of kyuubi trino server
  [KYUUBI apache#3081][DOCS] Add Hudi connector doc in Trino
  [KYUUBI apache#4357] Bump Jersey from 2.38 to 2.39
  [KYUUBI apache#4338][FOLLOWUP] Fix K8s integration tests
  [KYUUBI apache#4326] [ARROW] Fix Spark session timezone format in arrow-based result format
  [KYUUBI apache#4360][FOLLOWUP] Get valid unlimited users from existing limiters instead of conf
  [KYUUBI apache#4362] Add `_configurations` in kerberos.rst
  [KYUUBI apache#4338] Bump Spark from 3.3.1 to 3.3.2
  [KYUUBI apache#4119][FOLLOWUP] Add app start time for batch api docs
  ...
yanghua pushed a commit to awesome-kyuubi/kyuubi that referenced this pull request Apr 25, 2023
@turboFei @yanghua
[KYUUBI apache#4372] Support to return null value for OperationsResou…
e4be464 
…rce rowset

### _Why are the changes needed?_

With restful api, for query `select cast(null as int) c1`

#### Before
the result is `0`

#### After
the result is `null`.
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes apache#4372 from turboFei/null_value.

Closes apache#4372

b15f1eb [fwang12] nit
5f16855 [fwang12] fix
45c60dd [fwang12] check is set

Authored-by: fwang12 <fwang12@ebay.com>
Signed-off-by: fwang12 <fwang12@ebay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenliang123 bowenliang123 bowenliang123 approved these changes

@pan3793 pan3793 pan3793 approved these changes

Assignees

@turboFei turboFei

Labels
kind:bug This is a clearly a bug module:server
Projects
None yet
Milestone
v1.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@turboFei @codecov-commenter @bowenliang123 @pan3793