Complete SQL array literal support for logical types (#19338) - #19586
AnkitaAdvitot wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19586 +/- ##
============================================
- Coverage 67.82% 67.82% -0.01%
Complexity 1450 1450
============================================
Files 3502 3502
Lines 226479 226494 +15
Branches 35790 35799 +9
============================================
+ Hits 153613 153619 +6
- Misses 60749 60758 +9
Partials 12117 12117
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
| return bytesArr; | ||
| } | ||
| if (clazz == Timestamp.class) { |
There was a problem hiding this comment.
Could we keep the branches in the usual type order? That would put BIG_DECIMAL before BOOLEAN and TIMESTAMP before STRING and BYTES, with UUID last.
There was a problem hiding this comment.
Updated the branches in arrayValueConstructor to follow the standard Pinot type order: BIG_DECIMAL, BOOLEAN, TIMESTAMP, STRING, BYTES, and UUID last. Also updated ArrayFunctionsTest accordingly.
| case BOOLEAN: | ||
| Preconditions.checkState(singleValue, "Boolean array is not supported"); | ||
| return PinotDataType.BOOLEAN; | ||
| return singleValue ? PinotDataType.BOOLEAN |
There was a problem hiding this comment.
Could we move BOOLEAN after BIG_DECIMAL so this switch follows INT, LONG, FLOAT, DOUBLE, BIG_DECIMAL, BOOLEAN, TIMESTAMP, STRING, UUID? The separate BYTES validation can stay above the switch.
There was a problem hiding this comment.
Updated LiteralContext.getPinotDataType so that the switch follows INT, LONG, FLOAT, DOUBLE, BIG_DECIMAL, BOOLEAN, TIMESTAMP, STRING, UUID, keeping the separate BYTES validation above the switch. Also aligned LiteralContext.toString() with this order.
| {DataType.BYTES, new byte[]{1, 2}, new byte[]{1, 2}, new byte[]{1, 3}}, | ||
| {DataType.BYTES, new byte[][]{{1}, {2}}, new byte[][]{{1}, {2}}, new byte[][]{{1}, {3}}} | ||
| {DataType.BYTES, new byte[][]{{1}, {2}}, new byte[][]{{1}, {2}}, new byte[][]{{1}, {3}}}, | ||
| {DataType.BOOLEAN, new boolean[]{true, false}, new boolean[]{true, false}, new boolean[]{true, true}}, |
There was a problem hiding this comment.
Could we order these new cases consistently with the production type order: BIG_DECIMAL, BOOLEAN, TIMESTAMP, then the existing STRING and BYTES cases, and UUID last?
There was a problem hiding this comment.
Reordered the cases in LiteralContextTest.arrayBackedValues() and the corresponding test methods to match the standard production order (BIG_DECIMAL, BOOLEAN, TIMESTAMP, STRING, BYTES, UUID), and added testTimestampLiteral for full branch coverage.
| case BOOLEAN: | ||
| Preconditions.checkState(singleValue, "Boolean array is not supported"); | ||
| return PinotDataType.BOOLEAN; | ||
| return singleValue ? PinotDataType.BOOLEAN |
There was a problem hiding this comment.
MAJOR: Logical array literals still fail in single-stage queries. LiteralContext now accepts BOOLEAN, BIG_DECIMAL, TIMESTAMP, and UUID arrays, but TransformFunctionFactory routes array literals to ArrayLiteralTransformFunction, whose constructors support none of these types. For example, ARRAY[TRUE, FALSE] reaches its unsupported-type branch. Please extend that execution path and cover it with a query-level regression test.
There was a problem hiding this comment.
Extended ArrayLiteralTransformFunction to support BOOLEAN, BIG_DECIMAL, TIMESTAMP, and UUID in both constructors (LiteralContext and List<ExpressionContext>), implemented transformToBigDecimalValuesMV, and added cross-type MV conversions. In addition, prevented CompileTimeFunctionsInvoker from folding arrayValueConstructor into unsupported Thrift literals, ensuring the logical array types are preserved and evaluated by ArrayLiteralTransformFunction. Added unit tests in ArrayLiteralTransformFunctionTest and an end-to-end query regression test in TransformQueriesTest.testArrayLiteralQueries.
| put(Timestamp[].class, ColumnDataType.TIMESTAMP_ARRAY); | ||
| put(String[].class, ColumnDataType.STRING_ARRAY); | ||
| put(byte[][].class, ColumnDataType.BYTES_ARRAY); | ||
| put(UUID[].class, ColumnDataType.UUID_ARRAY); |
There was a problem hiding this comment.
MAJOR: This maps a UUID[] scalar-function result to UUID_ARRAY, but the single-stage ScalarTransformFunctionWrapper has no transformToBytesValuesMV implementation for that result; BaseTransformFunction throws when the result is read. FunctionUtils.getRelDataType also lacks UUID_ARRAY, so planner and executor types disagree. Please complete both paths before registering UUID[] as a supported return type.
There was a problem hiding this comment.
Added UUID_ARRAY in FunctionUtils.getRelDataType to return ARRAY<UUID> so planner and executor agree. In ScalarTransformFunctionWrapper, implemented transformToBytesValuesMV (supporting BYTES_ARRAY and UUID_ARRAY) as well as transformToBigDecimalValuesMV, and added UUID_ARRAY deserialization in getNonLiteralValues. Added unit tests in FunctionUtilsTest and ScalarTransformFunctionWrapperTest.
…types - Reorder type branches in ArrayFunctions.arrayValueConstructor to standard Pinot type order (BIG_DECIMAL, BOOLEAN, TIMESTAMP, STRING, BYTES, UUID) - Reorder switch branches in LiteralContext.getPinotDataType and test cases in LiteralContextTest, and add testTimestampLiteral - Extend ArrayLiteralTransformFunction to support BOOLEAN, BIG_DECIMAL, TIMESTAMP, and UUID in both constructors and MV transform conversions - Avoid folding arrayValueConstructor in CompileTimeFunctionsInvoker to preserve array literal logical types in single-stage engine - Register UUID[].class and UUID_ARRAY in FunctionUtils, and implement transformToBytesValuesMV and UUID_ARRAY conversion in ScalarTransformFunctionWrapper - Add unit and query-level regression tests across pinot-common and pinot-core
PR flow
Flow for boolean array literal: LiteralContext type detection → ArrayLiteralTransformFunction constructor → transformToIntValuesMV using internal int array.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Diff evidence
Description
Fixes #19338.
This is a follow-up to #19247 addressing the remaining gaps for SQL array literals and logical types:
ArrayFunctions.arrayValueConstructor: Added handling forTimestampandUUIDelements so that typedTimestamp[]andUUID[]arrays are returned instead of falling back to genericObject[].LiteralContext: Enabled multi-value array literal support for logical typesBOOLEAN(boolean[]andBoolean[]),BIG_DECIMAL(BigDecimal[]),TIMESTAMP(Timestamp[]), andUUID(UUID[]).LiteralContext.toString(): Added string formatting representations forPRIMITIVE_BOOLEAN_ARRAY,BOOLEAN_ARRAY,BIG_DECIMAL_ARRAY,TIMESTAMP_ARRAY, andUUID_ARRAY.FunctionUtils: RegisteredUUID[].classmapping toColumnDataType.UUID_ARRAYinCOLUMN_DATA_TYPE_MAP.Validation
pinot-common:LiteralContextTest,ArrayFunctionsTest,FunctionUtilsTest,LiteralSerDeTest(all 48 passed).mvn checkstyle:check).