Add all supported types to return type inference from string literal for JSON extract functions in the multi-stage query engine #14289

yashmayya · 2024-10-24T06:03:38Z

Currently, a query like SELECT JSON_EXTRACT_SCALAR(payload_commits, '$[*].author.name', 'STRING_ARRAY') FROM github_events fails on the multi-stage query engine with an error like: Invalid type: STRING_ARRAY.
This is because the type inference from string literals in TransformFunctionType doesn't cover all the types supported by JSON_EXTRACT_SCALAR / JSON_EXTRACT_INDEX.

This patch fixes the above issue by covering all the supported types taken from here -

pinot/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java

Lines 101 to 109 in 7879e07

    
           DataType dataType; 
        
           try { 
        
             dataType = DataType.valueOf(isSingleValue ? resultsType : resultsType.substring(0, resultsType.length() - 6)); 
        
           } catch (Exception e) { 
        
             throw new IllegalArgumentException(String.format( 
        
                 "Unsupported results type: %s for jsonExtractScalar function. Supported types are: " 
        
                     + "INT/LONG/FLOAT/DOUBLE/BOOLEAN/BIG_DECIMAL/TIMESTAMP/STRING/INT_ARRAY/LONG_ARRAY/FLOAT_ARRAY" 
        
                     + "/DOUBLE_ARRAY/STRING_ARRAY", resultsType)); 
        
           }

.

…for JSON extract functions in the multi-stage query engine

codecov-commenter · 2024-10-24T06:41:06Z

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.77%. Comparing base (59551e4) to head (b987037).
Report is 1267 commits behind head on master.

Files with missing lines	Patch %	Lines
...e/pinot/common/function/TransformFunctionType.java	85.71%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14289      +/-   ##
============================================
+ Coverage     61.75%   63.77%   +2.02%     
- Complexity      207     1556    +1349     
============================================
  Files          2436     2660     +224     
  Lines        133233   145837   +12604     
  Branches      20636    22313    +1677     
============================================
+ Hits          82274    93012   +10738     
- Misses        44911    45961    +1050     
- Partials       6048     6864     +816

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.75% <85.71%> (+2.04%)`	⬆️
java-21	`63.66% <85.71%> (+2.04%)`	⬆️
skip-bytebuffers-false	`63.76% <85.71%> (+2.02%)`	⬆️
skip-bytebuffers-true	`63.65% <85.71%> (+35.92%)`	⬆️
temurin	`63.77% <85.71%> (+2.02%)`	⬆️
unittests	`63.77% <85.71%> (+2.02%)`	⬆️
unittests1	`55.47% <85.71%> (+8.57%)`	⬆️
unittests2	`34.16% <0.00%> (+6.43%)`	⬆️

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.

bziobrowski · 2024-10-30T13:15:41Z

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

  }

  private static RelDataType inferTypeFromStringLiteral(String operandTypeStr, RelDataTypeFactory typeFactory) {
    switch (operandTypeStr) {


Shouldn't the list also contain:

BOOLEAN

TIMESTAMP

FLOAT

DOUBLE
?

Those are handled in the default branch.

bziobrowski · 2024-10-30T13:17:59Z

pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java

            "SELECT /*+ aggOptions(is_skip_leaf_stage_group_by='true') */ a.col2, a.col3 FROM a JOIN b "
                + "ON a.col1 = b.col1  WHERE a.col3 >= 0 GROUP BY a.col2, a.col3"
        },
-        new Object[]{"SELECT ROUND(ts_timestamp, 10000) FROM a"}


I think it'd be good add tests for other result types.

Jackie-Jiang

Good catch!

In Calcite, FLOAT is a synonym for DOUBLE (see here), and we'll need to map FLOAT in pinot to REAL.
Suggest adding a test to verify all supported types

yashmayya · 2024-10-31T05:08:16Z

In Calcite, FLOAT is a synonym for DOUBLE (see here), and we'll need to map FLOAT in pinot to REAL

Ah yeah, I did do this for FLOAT_ARRAY, but missed adding it for FLOAT.

Suggest adding a test to verify all supported types

I've added test cases for all supported types (from here) to QueryCompilationTest which will verify that the Calcite type inference is valid and that the query compiles. There's already existing tests for the transform function itself so I don't think we need to validate that separately for the multi-stage engine.

Add all supported types to return type inference from string literal …

e88d83f

…for JSON extract functions in the multi-stage query engine

yashmayya added bugfix multi-stage Related to the multi-stage query engine labels Oct 24, 2024

yashmayya requested a review from Jackie-Jiang October 24, 2024 06:03

bziobrowski reviewed Oct 30, 2024

View reviewed changes

Jackie-Jiang reviewed Oct 30, 2024

View reviewed changes

Add case for FLOAT; add test cases for all supported types

b987037

Jackie-Jiang approved these changes Oct 31, 2024

View reviewed changes

Jackie-Jiang merged commit e61d503 into apache:master Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add all supported types to return type inference from string literal for JSON extract functions in the multi-stage query engine #14289

Add all supported types to return type inference from string literal for JSON extract functions in the multi-stage query engine #14289

Uh oh!

yashmayya commented Oct 24, 2024

Uh oh!

codecov-commenter commented Oct 24, 2024 •

edited

Loading

Uh oh!

bziobrowski Oct 30, 2024

Uh oh!

yashmayya Oct 31, 2024

Uh oh!

bziobrowski Oct 30, 2024

Uh oh!

Jackie-Jiang left a comment

Uh oh!

yashmayya commented Oct 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	DataType dataType;
	try {
	dataType = DataType.valueOf(isSingleValue ? resultsType : resultsType.substring(0, resultsType.length() - 6));
	} catch (Exception e) {
	throw new IllegalArgumentException(String.format(
	"Unsupported results type: %s for jsonExtractScalar function. Supported types are: "
	+ "INT/LONG/FLOAT/DOUBLE/BOOLEAN/BIG_DECIMAL/TIMESTAMP/STRING/INT_ARRAY/LONG_ARRAY/FLOAT_ARRAY"
	+ "/DOUBLE_ARRAY/STRING_ARRAY", resultsType));
	}

Add all supported types to return type inference from string literal for JSON extract functions in the multi-stage query engine #14289

Add all supported types to return type inference from string literal for JSON extract functions in the multi-stage query engine #14289

Uh oh!

Conversation

yashmayya commented Oct 24, 2024

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

bziobrowski Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

yashmayya Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

bziobrowski Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

yashmayya commented Oct 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Oct 24, 2024 •

edited

Loading