Support SET key=value syntax #9017

walterddr · 2022-07-05T17:27:02Z

Background

context: #8906

Description

This PR introduces a new way to set environmental options for executing a SQL query on Pinot.

Syntax examples for single and multiple environment options:

SET skipUpsert = 'false';
SELECT count(*) FROM tbl

SET foo = 'bar';
SET some_key = 'some_value';
SELECT * FROM tbl

Details

; at the end of the SET statement indicate it is an end of a statement; However, Pinot doesn't support multiple statements, thus all SET statement(s) are only valid within the SQL query string context.
- this means first executing ALTER SESSION SET key = 'value'; and then executing another query won't affect the query.
- this also means, effectively one pinot query string is one individual, isolated user session.
we allow multiple SET statements within a single query string

Jackie-Jiang

LGTM otherwise

pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java

Jackie-Jiang · 2022-07-11T23:16:27Z

pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java

+    return options;
+  }
+
+  private static void setOptions(PinotQuery pinotQuery, List<String> optionsStatements) {


Is this still used?

yes it is for backward compatibility will remove once the OPTION_REGEX is removed.

pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java

codecov-commenter · 2022-07-12T02:47:24Z

Codecov Report

Merging #9017 (c260a3e) into master (de16a0a) will decrease coverage by 3.23%.
The diff coverage is 74.32%.

❗ Current head c260a3e differs from pull request most recent head 5b7c51c. Consider uploading reports for the commit 5b7c51c to get more accurate results

@@             Coverage Diff              @@
##             master    #9017      +/-   ##
============================================
- Coverage     70.08%   66.85%   -3.24%     
+ Complexity     4957     4766     -191     
============================================
  Files          1827     1369     -458     
  Lines         96064    69842   -26222     
  Branches      14356    10912    -3444     
============================================
- Hits          67327    46693   -20634     
+ Misses        24092    19864    -4228     
+ Partials       4645     3285    -1360

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`66.85% <74.32%> (-0.02%)`	⬇️
unittests2	`?`

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

Impacted Files	Coverage Δ
...g/apache/pinot/common/metrics/ControllerMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java	`77.77% <ø> (-0.31%)`	⬇️
...local/indexsegment/mutable/MutableSegmentImpl.java	`54.34% <42.85%> (-0.56%)`	⬇️
...org/apache/pinot/sql/parsers/CalciteSqlParser.java	`86.41% <77.77%> (-1.56%)`	⬇️
.../pinot/common/restlet/resources/TableTierInfo.java	`100.00% <100.00%> (ø)`
...rg/apache/pinot/sql/parsers/SqlNodeAndOptions.java	`100.00% <100.00%> (ø)`
...g/apache/pinot/sql/parsers/dml/InsertIntoFile.java	`85.18% <100.00%> (ø)`
...va/org/apache/pinot/core/routing/RoutingTable.java	`0.00% <0.00%> (-100.00%)`	⬇️
...va/org/apache/pinot/common/config/NettyConfig.java	`0.00% <0.00%> (-100.00%)`	⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 700 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de16a0a...5b7c51c. Read the comment docs.

siddharthteotia · 2022-07-12T15:41:25Z

pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java

    }
+    if (sqlType == null) {
+      throw new SqlCompilationException("SqlNode with statement not found!");


Should we check that there has to be exactly 1 query/DQL statement unless it was an INSERT call ?

SET skipUpsert = 'false';

The above has no query statement but just the option SET which by alone is meaningless ?

siddharthteotia · 2022-07-12T15:45:13Z

pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java

+    Assert.assertEquals(pinotQuery.getQueryOptions().get("foo"), "1234");
+    Assert.assertEquals(pinotQuery.getQueryOptions().get("bar"), "'potato'");
+
+    // test invalid options


Can you also add a test where you have 2 DQL statements and 0 or more SET statements ?

Rong Rong added 2 commits July 5, 2022 10:14

support SET OPTIONS query syntax

6e96482

add back deprecated OPTIONS keyword support, but clearly warn

60a49a0

walterddr requested review from xiangfu0, Jackie-Jiang and siddharthteotia July 7, 2022 17:48

Jackie-Jiang approved these changes Jul 11, 2022

View reviewed changes

walterddr commented Jul 12, 2022

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java Outdated Show resolved Hide resolved

Apply suggestions from code review

5b7c51c

siddharthteotia reviewed Jul 12, 2022

View reviewed changes

siddharthteotia approved these changes Jul 12, 2022

View reviewed changes

address diff comments

b05cf2b

walterddr merged commit 0b657cf into apache:master Jul 12, 2022

walterddr deleted the query_option_using_set branch December 6, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SET key=value syntax #9017

Support SET key=value syntax #9017

walterddr commented Jul 5, 2022 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Jul 11, 2022

walterddr Jul 12, 2022

codecov-commenter commented Jul 12, 2022

siddharthteotia Jul 12, 2022 •

edited

Loading

siddharthteotia Jul 12, 2022

Support SET key=value syntax #9017

Support SET key=value syntax #9017

Conversation

walterddr commented Jul 5, 2022 • edited Loading

Background

Description

Details

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang Jul 11, 2022

Choose a reason for hiding this comment

walterddr Jul 12, 2022

Choose a reason for hiding this comment

codecov-commenter commented Jul 12, 2022

Codecov Report

siddharthteotia Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

siddharthteotia Jul 12, 2022

Choose a reason for hiding this comment

walterddr commented Jul 5, 2022 •

edited

Loading

siddharthteotia Jul 12, 2022 •

edited

Loading