Skip to content

Fix literal type handling to be SQL compatible#11749

Closed
Jackie-Jiang wants to merge 2 commits intoapache:masterfrom
Jackie-Jiang:literal_type_handling
Closed

Fix literal type handling to be SQL compatible#11749
Jackie-Jiang wants to merge 2 commits intoapache:masterfrom
Jackie-Jiang:literal_type_handling

Conversation

@Jackie-Jiang
Copy link
Copy Markdown
Contributor

@Jackie-Jiang Jackie-Jiang commented Oct 6, 2023

Currently when literal is single quoted (e.g. 'abc', '123', '1.23'), the type is auto-derived based on the literal itself. Also, it can only derive to big-decimal, timestamp or string but not other numeric types.
In Standard SQL (we reference PostgreSQL convention as standard), single quoted literal has unknown type and the type should be resolved based on the context around the literal (read more here).

This PR:

  • Preserve the literal type, and only resolve the type when the context is clear
  • Fix the wrong behavior when quoted string is used as number (currently it will be resolved as 0)
  • Fix CASE to follow the PostgreSQL convention (read more here), and fix a bug of reading float value as double type
  • Fix literal toString() to properly reflect the literal type

Deployment

Must deploy broker before server (following the deployment convention)

@Jackie-Jiang Jackie-Jiang added enhancement Improvement to existing functionality incompatible release-notes Referenced by PRs that need attention when compiling the next release notes labels Oct 6, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 6, 2023

Codecov Report

❌ Patch coverage is 87.46736% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.22%. Comparing base (13c2901) to head (adaab9f).
⚠️ Report is 4234 commits behind head on master.

Files with missing lines Patch % Lines
...ator/transform/function/CaseTransformFunction.java 86.66% 8 Missing and 4 partials ⚠️
...e/pinot/common/request/context/LiteralContext.java 88.42% 8 Missing and 3 partials ⚠️
...org/apache/pinot/common/utils/http/HttpClient.java 0.00% 5 Missing ⚠️
...apache/pinot/common/utils/SqlResultComparator.java 0.00% 4 Missing ⚠️
...unction/IntegerTupleSketchAggregationFunction.java 0.00% 4 Missing ⚠️
...t/core/query/selection/SelectionOperatorUtils.java 96.29% 0 Missing and 4 partials ⚠️
...gregation/function/AggregationFunctionFactory.java 50.00% 0 Missing and 3 partials ⚠️
...pache/pinot/common/utils/request/RequestUtils.java 87.50% 0 Missing and 1 partial ⚠️
...ot/core/operator/filter/H3IndexFilterOperator.java 50.00% 1 Missing ⚠️
...re/query/reduce/SelectionOnlyStreamingReducer.java 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #11749      +/-   ##
============================================
+ Coverage     63.13%   63.22%   +0.08%     
- Complexity     1117     1118       +1     
============================================
  Files          2342     2342              
  Lines        125916   126017     +101     
  Branches      19370    19409      +39     
============================================
+ Hits          79501    79675     +174     
+ Misses        40749    40675      -74     
- Partials       5666     5667       +1     
Flag Coverage Δ
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 63.18% <87.46%> (+0.10%) ⬆️
java-17 63.08% <87.46%> (+0.13%) ⬆️
java-20 63.06% <87.46%> (+0.07%) ⬆️
temurin 63.22% <87.46%> (+0.08%) ⬆️
unittests 63.22% <87.46%> (+0.08%) ⬆️
unittests1 67.39% <87.46%> (+0.16%) ⬆️
unittests2 14.42% <0.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jackie-Jiang Jackie-Jiang force-pushed the literal_type_handling branch 3 times, most recently from 022b0bf to a67df16 Compare October 6, 2023 08:15
@Jackie-Jiang
Copy link
Copy Markdown
Contributor Author

Use int literal is backward incompatible because server side expects long literal for int values. This PR fixes the server side to support both int and long literal. Tracking the changes required for int literal in #11751

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why transient? This class is not serializable, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct. Added transient for documentation purpose and future proof because it is not included in equals() and hashCode(). Certain tools such as EqualsVerifier won't pass if a non-transient field is not used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I would prefer javadoc comments. They are included in IDE documentation, while single line comments are not

@Jackie-Jiang Jackie-Jiang force-pushed the literal_type_handling branch 2 times, most recently from 1d98d7a to 2fa9e7f Compare October 6, 2023 20:55
@Jackie-Jiang Jackie-Jiang force-pushed the literal_type_handling branch 2 times, most recently from 4726b82 to 75d590f Compare October 7, 2023 08:05
@Jackie-Jiang Jackie-Jiang force-pushed the literal_type_handling branch from 75d590f to adaab9f Compare October 8, 2023 07:31
@Jackie-Jiang
Copy link
Copy Markdown
Contributor Author

Jackie-Jiang commented Oct 8, 2023

This PR has become too large. Breaking this PR into smaller PRs and keep it for future reference:

  1. Literal type handling: [SQL Compatible] Fix literal type handling #11763
  2. Broker side handling on expression format change: Enhance Broker reducer to handle expression format change #11762
  3. Change LiteralContext.toString() to reflect the literal type (requires broker side handling, so probably after the next release)
  4. Use INT type for int literal (after the next release)

@Jackie-Jiang Jackie-Jiang deleted the literal_type_handling branch October 9, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants