Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add syntax support for PARTITIONED BY/CLUSTERED BY in INSERT queries #12163

Merged
merged 54 commits into from
Feb 8, 2022
Merged

Add syntax support for PARTITIONED BY/CLUSTERED BY in INSERT queries #12163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
29969da
Initial commit, build working
LakshSingla Jan 4, 2022
6d3382f
Merge branch 'master' into calcite-ext-prototype
LakshSingla Jan 4, 2022
f70aad9
Add for SqlSelect and OrderBy
LakshSingla Jan 4, 2022
7b2c884
Integrate the changes such that set context works
LakshSingla Jan 6, 2022
f406334
Merge branch 'master' into calcite-ext-prototype
LakshSingla Jan 6, 2022
c9d9bc9
Fix nullptr in DruidPlanner
LakshSingla Jan 6, 2022
ab57bc0
Add files for SqlInsert
LakshSingla Jan 17, 2022
7a3e479
Revert setcontext changes, only insert changes being exposed
LakshSingla Jan 17, 2022
d791669
Cleanup code, ClusterBy will create a new SqlNode of type OrderBy
LakshSingla Jan 17, 2022
5b741a6
Cleanup code, revert unnesecary changes
LakshSingla Jan 17, 2022
2ddad66
Merge branch 'master' into calcite-ext-insert
LakshSingla Jan 17, 2022
1845ddf
Remove contextMap from ParsedNodes
LakshSingla Jan 17, 2022
2817a85
Add getter for partitionBy
LakshSingla Jan 17, 2022
7d63e44
Checkstyle fix
LakshSingla Jan 17, 2022
a22eb0e
Add generated files to ignorelist of forbiddenapis
LakshSingla Jan 17, 2022
1119dd0
indentation fix, add license to insert.ftl
LakshSingla Jan 18, 2022
50ce3fb
Add testcases which plan using modified syntax
LakshSingla Jan 18, 2022
472291c
Handle LIMIT/OFFSET clauses, add more testcases
LakshSingla Jan 18, 2022
6c2fcf2
Exclude generated classes from spotbug/forbiddenapis
LakshSingla Jan 19, 2022
e31ebcc
Merge branch 'master' into calcite-ext-insert
LakshSingla Jan 19, 2022
0b7dec6
Remove version from mvn-dep plugin, add sl4j dep
LakshSingla Jan 19, 2022
f78606c
Pass the partition by granularity in the querycontext
LakshSingla Jan 24, 2022
4768209
Merge branch 'master' into calcite-ext-insert
LakshSingla Jan 26, 2022
c290a63
Address review comments, check on the granularity passed
LakshSingla Jan 26, 2022
4f4cb77
Improve testcase for cluster by
LakshSingla Jan 26, 2022
8371400
Fix build, add CLUSTER to kwds as well
LakshSingla Jan 26, 2022
94d9175
fix format string
LakshSingla Jan 27, 2022
585cd61
Make CLUSTER reserved keyword
LakshSingla Jan 27, 2022
1e54241
POM changes - move plugin version to parent, scope=provided for slf4j
LakshSingla Jan 28, 2022
4330703
Rename cluster to clustered, partition to partitioned
LakshSingla Jan 28, 2022
0ae5570
Disallow ORDER BY in SELECT inside INSERT queries
LakshSingla Jan 28, 2022
37728f2
Indent ftl properly, try cleanup around Insert and DruidSqlInsert
LakshSingla Jan 28, 2022
4e63d27
checkstyle fix
LakshSingla Jan 28, 2022
1b62e6c
Update the partition by clause
LakshSingla Feb 2, 2022
960870a
Update the error messages in parsing class
LakshSingla Feb 2, 2022
8717d0b
semicolon fix, javadoc
LakshSingla Feb 2, 2022
72ae0bd
Add test cases for the util class
LakshSingla Feb 2, 2022
5cbcb35
checkstyle fix
LakshSingla Feb 2, 2022
38aa941
Partitioned by now a required clause
LakshSingla Feb 3, 2022
7df0610
Update testcases, minor review comments
LakshSingla Feb 3, 2022
d8d57f4
fix test cases
LakshSingla Feb 3, 2022
d36bc2b
add tests, cleanup error message
LakshSingla Feb 3, 2022
eaaad27
Change the value of insert segment granularity in querycontext
LakshSingla Feb 3, 2022
b90ab16
minor fixes
LakshSingla Feb 3, 2022
e335214
intellij inspections
LakshSingla Feb 4, 2022
2cef674
Exclude druidsqlinsert from codecov
LakshSingla Feb 4, 2022
8d58136
Exclude druidsqlinsert from codecov2
LakshSingla Feb 4, 2022
973d3fa
Missed testcase
LakshSingla Feb 7, 2022
dc2b45e
DruidSqlInsert.unparse working
LakshSingla Feb 7, 2022
32244a6
Fix error messages
LakshSingla Feb 7, 2022
5017257
Revert codecov exclude
LakshSingla Feb 7, 2022
5b79694
reduce severity for parsing error log
LakshSingla Feb 7, 2022
43b7388
intellij, test errors
LakshSingla Feb 8, 2022
ad78bba
unused import
LakshSingla Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
minor fixes
  • Loading branch information
@LakshSingla
LakshSingla committed Feb 3, 2022
commit b90ab169b9163d9797b3280dc7c0dac8ac46a20f
2 changes: 1 addition & 1 deletion sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public void unparse(SqlWriter writer, int leftPrec, int rightPrec)
{
super.unparse(writer, leftPrec, rightPrec);
writer.keyword("PARTITIONED BY");
writer.keyword(getPartitionedBy().toString()); // TODO: Can this be made cleaner
writer.keyword(getPartitionedBy().toString()); // TODO: Make it cleaner by directly unparsing the SqlNode
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this unparse method get used anywhere? If so, we need to fix this because it's going to be incorrect in a bunch of cases. Or if it doesn't get used anywhere, IMO it's better to throw UnsupportedOperationException rather than have a broken implementation. At least that way it's clear that the method isn't reliable.

Copy link
Contributor Author

@LakshSingla LakshSingla Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Updated the method, with the proper implementation. Here are a few unparse examples after the change:

INSERT INTO `dst`
(SELECT *
FROM `foo`) PARTITIONED BY ALL TIME

INSERT INTO `dst`
(SELECT *
FROM `view`.`aview`) PARTITIONED BY `TIME_FLOOR`(`__TIME`, 'PT2H')

INSERT INTO `druid`.`dst`
(SELECT `__time`, FLOOR(`m1`) AS `floor_m1`, `dim1`, CEIL(`m2`)
FROM `foo`) PARTITIONED BY FLOOR(`__TIME` TO DAY) CLUSTERED BY 2 `dim1` DESC CEIL(`m2`)

unparse method is important since this is being used in SqlNode's toString method. So it gets used while debugging to view the original node as a SQL string. (Also, it gets used in other unparse methods, so not relevant to DruidSqlInsert currently since it's a top-level node, but it might cause say Explain on a DriudSqlInsert to fail, which is going to be implemented). Therefore I would say it is important to get it right.

if (getClusteredBy() != null) {
writer.sep("CLUSTERED BY");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why sep rather than keyword? sep is supposed to be for list separators, which doesn't seem right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, oversight on my part

SqlWriter.Frame frame = writer.startList("", "");
Expand Down
4 changes: 2 additions & 2 deletions sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,15 @@ public static Granularity convertSqlNodeToGranularityThrowingParseExceptions(Sql
* This method is used to extract the granularity from a SqlNode representing following function calls:
* 1. FLOOR(__time TO TimeUnit)
* 2. TIME_FLOOR(__time, 'PT1H')
* <p>
*
* Validation on the sqlNode is contingent to following conditions:
* 1. sqlNode is an instance of SqlCall
* 2. Operator is either one of TIME_FLOOR or FLOOR
* 3. Number of operands in the call are 2
* 4. First operand is a SimpleIdentifier representing __time
* 5. If operator is TIME_FLOOR, the second argument is a literal, and can be converted to the Granularity class
* 6. If operator is FLOOR, the second argument is a TimeUnit, and can be mapped using {@link TimeUnits}
* <p>
*
* Since it is to be used primarily while parsing the SqlNode, it is wrapped in {@code convertSqlNodeToGranularityThrowingParseExceptions}
*
* @param sqlNode SqlNode representing a call to a function
Expand Down