Skip to content

Conversation

@machavan
Copy link
Contributor

@machavan machavan commented Feb 11, 2025

This fix of checking for null value of a DTVImpl before calling DTVImpl::getValue has shown some perf gains in our internal benchmark.

This fix will change behavior of getBinaryStream /getAsciiStream in ResultSet to be compliant with JDBC specs:
a Java input stream that delivers the database column value as a stream of one-byte ASCII characters; if the value is SQL NULL, the value returned is null

@machavan machavan changed the title Perf fix 1 - Add a null value check upfront in dtv::getValue Perf fix 1 - Add a null value check upfront in DTV::getValue Feb 11, 2025
@codecov
Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.33%. Comparing base (e63814e) to head (01e79e9).
Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2600      +/-   ##
============================================
- Coverage     51.35%   51.33%   -0.03%     
+ Complexity     3981     3970      -11     
============================================
  Files           147      147              
  Lines         33686    33688       +2     
  Branches       5627     5628       +1     
============================================
- Hits          17300    17294       -6     
+ Misses        13956    13953       -3     
- Partials       2430     2441      +11     

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

@Jeffery-Wasty Jeffery-Wasty added this to the 12.10.0 milestone Feb 11, 2025
@Jeffery-Wasty Jeffery-Wasty linked an issue Feb 11, 2025 that may be closed by this pull request
Copy link
Contributor

@lilgreenbird lilgreenbird left a comment

Choose a reason for hiding this comment

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

add test?

@muskan124947
Copy link
Contributor

@machavan
Copy link
Contributor Author

add test?

Verified that existing tests already cover this code path. This code path is hit for any get call on a ResultSet that returns NULL. e.g. many tests in ResultSetTest cover this.

@muskan124947 muskan124947 merged commit b0ff5b1 into main Feb 20, 2025
18 of 19 checks passed
@muskan124947 muskan124947 deleted the dev/machavan/perffix1 branch February 20, 2025 08:38
@lilgreenbird lilgreenbird changed the title Perf fix 1 - Add a null value check upfront in DTV::getValue Check for null when getting DTV values (java compliance - getAsciiStream will return null when the value is null) Feb 27, 2025
@lilgreenbird lilgreenbird changed the title Check for null when getting DTV values (java compliance - getAsciiStream will return null when the value is null) Check for null when getting DTV values (JDBC spec compliance - getAsciiStream will return null when the value is null) Feb 27, 2025
@lilgreenbird lilgreenbird changed the title Check for null when getting DTV values (JDBC spec compliance - getAsciiStream will return null when the value is null) Check for null when getting DTV values (JDBC spec compliance - getBinaryStream /getAsciiStream will return null when the value is null) Feb 27, 2025
@lilgreenbird
Copy link
Contributor

test added in PR #2617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed/Merged PRs

Development

Successfully merging this pull request may close these issues.

8 participants