[SQL Compatible] Fix literal type handling #11763

Jackie-Jiang · 2023-10-09T08:04:47Z

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

Incompatible

Following standard SQL, single quoted literal won't have its type auto-derived. In order to create big-decimal literal, user should use cast('12345.6789' AS DECIMAL) explicitly

codecov-commenter · 2023-10-09T08:47:10Z

Codecov Report

❌ Patch coverage is 84.98024% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.24%. Comparing base (aff30d6) to head (afd4c0e).
⚠️ Report is 3068 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.17%	8 Missing and 3 partials ⚠️
...org/apache/pinot/common/utils/http/HttpClient.java	0.00%	5 Missing ⚠️
...unction/IntegerTupleSketchAggregationFunction.java	0.00%	4 Missing ⚠️
...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 ⚠️
...ry/runtime/plan/server/ServerPlanRequestUtils.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #11763      +/-   ##
============================================
+ Coverage     63.08%   63.24%   +0.15%     
  Complexity     1140     1140              
============================================
  Files          2343     2343              
  Lines        126320   126335      +15     
  Branches      19423    19436      +13     
============================================
+ Hits          79694    79900     +206     
+ Misses        40953    40765     -188     
+ Partials       5673     5670       -3

Flag	Coverage Δ
integration	`<0.01% <0.00%> (ø)`
integration1	`<0.01% <0.00%> (ø)`
integration2	`0.00% <0.00%> (ø)`
java-11	`63.16% <84.98%> (+0.09%)`	⬆️
java-17	`63.08% <84.98%> (+0.12%)`	⬆️
java-20	`63.08% <84.98%> (+0.11%)`	⬆️
temurin	`63.24% <84.98%> (+0.15%)`	⬆️
unittests	`63.23% <84.98%> (+0.15%)`	⬆️
unittests1	`67.42% <84.98%> (+0.17%)`	⬆️
unittests2	`14.42% <0.00%> (+0.02%)`	⬆️

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.

pinot-spi/src/main/java/org/apache/pinot/spi/utils/BooleanUtils.java

walterddr · 2023-10-10T15:31:21Z

...ntegration-tests/src/test/java/org/apache/pinot/integration/tests/custom/GeoSpatialTest.java

instead of changing it we should add one more for -1 to make sure the results is actually considered as true

This test is for backward compatible where we used to allow 0 and 1. We never allow -1, so that shouldn't be needed. More tests are covered in LiteralContextText

pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java

walterddr · 2023-10-10T15:55:36Z

pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java

is this relevant?

Not directly relevant, but find this not able to return the error during debugging.

walterddr · 2023-10-10T16:01:02Z

pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java

if (sqlNumericLiteral.isExact()) { if (sqlNumericLiteral.isInteger()) { // long/int } else { // float/double } } else { // TODO support exact decimal // decimal }

This is incorrect. FLOAT/DOUBLE is not exact.

walterddr · 2023-10-10T16:02:45Z

...-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StPointFunction.java

ST_POINT is ANY on 3rd argument, should it be boolean or integer?

It should be boolean. We allow int for backward compatibility only because this function was introduced before BOOLEAN is supported

walterddr · 2023-10-10T16:05:57Z

...c/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java

i dont really think . can be added in function name percentile99.5(col) is not valid. (https://www.postgresql.org/docs/current/xfunc-sql.html)

This is for percentile(col, 99.5) which is allowed

walterddr · 2023-10-10T16:09:50Z

...runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java

is this even needed? we should handle it in RequestUtils

AFAIK the arrFloat[rowIdx] will point to the double signature,

once uncomment the TODO it will use the float signature?

This is needed. We cannot directly cast float to double because that will create wrong value (e.g 0.05f -> 0.05000000074505806), then cause wrong result. Added more notes

xiangfu0

Let's also put the incompatible items into PR descriptions.

xiangfu0 · 2023-10-13T23:12:26Z

...st/java/org/apache/pinot/core/operator/transform/function/AdditionTransformFunctionTest.java

    testTransformFunction(transformFunction, expectedBigDecimalValues);

    expression = RequestContextUtils.getExpression(
-        String.format("add(add(12,%s),%s,add(add(%s,%s),'12110.34556677889901122335678',%s),%s)", STRING_SV_COLUMN,


Does it mean we won't auto infer big decimal/ts values and enforce the cast always?

Correct. In standard SQL, literal cannot be auto inferred based on its value.

elonazoulay · 2024-02-26T23:41:04Z

This is great, thanks so much for fixing this!

Jackie-Jiang added enhancement incompatible Indicate PR that introduces backward incompatibility release-notes Referenced by PRs that need attention when compiling the next release notes bugfix labels Oct 9, 2023

Jackie-Jiang requested review from walterddr and xiangfu0 October 9, 2023 08:04

This was referenced Oct 9, 2023

[test]more geospatial v2 integration tests #11741

Merged

Fix literal type handling to be SQL compatible #11749

Closed

walterddr reviewed Oct 10, 2023

View reviewed changes

Jackie-Jiang added 2 commits October 13, 2023 15:06

[SQL Compatible] Fix literal type handling

e532d9f

Address comments

afd4c0e

Jackie-Jiang force-pushed the literal_handling branch from 031b253 to afd4c0e Compare October 13, 2023 22:55

xiangfu0 approved these changes Oct 14, 2023

View reviewed changes

walterddr approved these changes Oct 14, 2023

View reviewed changes

Jackie-Jiang merged commit e826a23 into apache:master Oct 14, 2023

Jackie-Jiang deleted the literal_handling branch October 14, 2023 07:05

mosabua mentioned this pull request Feb 15, 2024

1.0.1 release timeline #12428

Closed

Jackie-Jiang mentioned this pull request Feb 24, 2024

Dividing by a single quoted numeric literal has different results than an unquoted numeric literal #11676

Closed

Jackie-Jiang mentioned this pull request May 4, 2024

SQL compliance: allow INT and FLOAT literal #13078

Merged

Jackie-Jiang mentioned this pull request Jun 7, 2024

[DO NOT MERGE] Handle legacy query and log error #13326

Closed

[SQL Compatible] Fix literal type handling #11763

[SQL Compatible] Fix literal type handling #11763

Uh oh!

Conversation

Jackie-Jiang commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Incompatible

Uh oh!

codecov-commenter commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elonazoulay commented Feb 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Jackie-Jiang commented Oct 9, 2023 •

edited

Loading

codecov-commenter commented Oct 9, 2023 •

edited

Loading

xiangfu0 Oct 13, 2023 •

edited

Loading