Add replace statement to sql parser #12386

adarshsanjeev · 2022-04-01T09:59:49Z

Relevant Issue: #11929

Add custom replace statement to Druid SQL parser.
Edit DruidPlanner to convert relevant fields to Query Context.
Refactor common code with Insert statements to reuse them for replace where possible.

This PR has:

been self-reviewed.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.

sql/src/main/codegen/includes/replace.ftl

sql/src/main/codegen/includes/common.ftl

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java

sql/src/main/codegen/includes/replace.ftl

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java

sql/src/main/codegen/config.fmpp

sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java

sql/src/main/codegen/config.fmpp

sql/src/main/codegen/includes/common.ftl

sql/src/main/codegen/includes/replace.ftl

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java

sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java

LakshSingla

Changes and tests look LGTM. Some NIT comments. Also, since we are reusing a lot of Calcite's rules for column and table names, can you please confirm we are not allowing a superset of the actual grammar to pass through the syntax phase? (For example, where we have a column list, we only want the column name and not anything like col1 as alias1. My guess is not since you have initialized the constructor, so it should be fine)

sql/src/main/codegen/includes/replace.ftl

sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java

adarshsanjeev · 2022-04-26T04:59:33Z

since we are reusing a lot of Calcite's rules for column and table names, can you please confirm we are not allowing a superset of the actual grammar to pass through the syntax phase?

The reuse is taken from the standard SqlInsert which DruidSqlInsert wraps, that has been restricted by trimming some of the syntax as you noted, so even at worst, it shouldn't be more than what is currently allowed in INSERT syntax.

LakshSingla

LGTM

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java

sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java

abhishekagarwal87 · 2022-04-29T11:57:48Z

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java

+        case LESS_THAN:
+          columnValuePair = createColumnValuePair(sqlBasicCall.getOperandList(), dateTimeZone);
+          return new BoundDimFilter(columnValuePair.left, null, columnValuePair.right, null, true, null, null, StringComparators.NUMERIC);
+        default:


we should also add support for BETWEEN if possible.

Added support. One thing to note is that BETWEEN in sql is inclusive for the interval. This might lead to some strange queries in most use cases, like __time BETWEEN TIMESTAMP '2000-01-01' AND TIMESTAMP '2000-01-31 23:59:59.999'. Should we stick with convention or make the end time exclusive?

abhishekagarwal87 · 2022-04-29T12:22:42Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteReplaceDmlTest.java

+
+import static org.junit.Assert.assertEquals;
+
+public class CalciteReplaceDmlTest extends CalciteIngestionDmlTest


Please also add tests for negative scenarios where users are trying to use REPLACE in a way that is not supported.

There are tests for the following negative scenarios

Invalid or missing overwrite where clause

Unsupported operator in overwrite where

A column other than __time is used in the query

Misaligned partition in the query

With no partition spec

Interval is empty

Are there any other scenarios I have missed that I should add tests for?

Question - Is ORDER BY or any other similar clauses prohibited in the REPLACE statement?
And, If so, can do tests for those need to be added?

Added a test for this

sql/src/main/codegen/includes/common.ftl

sql/src/test/java/org/apache/druid/sql/calcite/CalciteIngestionDmlTest.java

sql/src/test/java/org/apache/druid/sql/calcite/CalciteReplaceDmlTest.java

sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlUnparseTest.java

abhishekagarwal87

+1 after CI

Added replace statement to sql parser

3c95700

adarshsanjeev changed the title ~~Added replace statement to sql parser (Work in progress)~~ Added replace statement to sql parser Apr 1, 2022

FrankChen021 added the Area - SQL label Apr 2, 2022

Updated parser and planner.

53422a8

adarshsanjeev marked this pull request as ready for review April 4, 2022 06:18

abhishekagarwal87 added the Area - Batch Ingestion label Apr 4, 2022

gianm self-requested a review April 4, 2022 22:09

rohangarg reviewed Apr 5, 2022

View reviewed changes

LakshSingla reviewed Apr 5, 2022

View reviewed changes

Address review comments

3bc62bf

adarshsanjeev changed the title ~~Added replace statement to sql parser~~ Add replace statement to sql parser Apr 6, 2022

adarshsanjeev added 4 commits April 21, 2022 18:49

Merge remote-tracking branch 'origin/master' into replace-sql-parser

d2cad17

Update replace syntax

f3042c7

Update interval alignment validation

f442133

Fix checkstyle

e3275d4

LakshSingla reviewed Apr 26, 2022

View reviewed changes

Address review comments

a6b2c03

LakshSingla approved these changes Apr 26, 2022

View reviewed changes

abhishekagarwal87 reviewed Apr 26, 2022

View reviewed changes

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java Show resolved Hide resolved

abhishekagarwal87 reviewed Apr 26, 2022

View reviewed changes

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java Show resolved Hide resolved

abhishekagarwal87 reviewed Apr 26, 2022

View reviewed changes

sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java Show resolved Hide resolved

adarshsanjeev added 3 commits April 27, 2022 15:14

Merge remote-tracking branch 'origin/master' into replace-sql-parser

f8d51f9

Update syntax as per latest proposal

4cdfb27

Update syntax

d3d3128

abhishekagarwal87 reviewed Apr 29, 2022

View reviewed changes

LakshSingla reviewed May 2, 2022

View reviewed changes

sql/src/main/codegen/includes/common.ftl Outdated Show resolved Hide resolved

Address review comments

5773737

LakshSingla reviewed May 2, 2022

View reviewed changes

Add comments and unit test

772d060

adarshsanjeev added 2 commits May 5, 2022 18:50

Address review comments

4a61345

Add unit test

d300073

abhishekagarwal87 approved these changes May 10, 2022

View reviewed changes

adarshsanjeev closed this May 11, 2022

adarshsanjeev reopened this May 11, 2022

Merge remote-tracking branch 'origin/master' into replace-sql-parser

e14ee90

abhishekagarwal87 merged commit 39b3487 into apache:master May 13, 2022

abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022

techdocsmith mentioned this pull request Aug 26, 2022

[Draft] 24.0 Release notes #12825

Closed

abhishekagarwal87 mentioned this pull request Sep 8, 2022

Test issue [Please ignore] #13055

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add replace statement to sql parser #12386

Add replace statement to sql parser #12386

adarshsanjeev commented Apr 1, 2022 •

edited

Loading

LakshSingla left a comment

adarshsanjeev commented Apr 26, 2022

LakshSingla left a comment

abhishekagarwal87 Apr 29, 2022

adarshsanjeev May 2, 2022

abhishekagarwal87 Apr 29, 2022

adarshsanjeev May 2, 2022

LakshSingla May 6, 2022

adarshsanjeev May 9, 2022

abhishekagarwal87 left a comment


		import static org.junit.Assert.assertEquals;

		public class CalciteReplaceDmlTest extends CalciteIngestionDmlTest

Add replace statement to sql parser #12386

Add replace statement to sql parser #12386

Conversation

adarshsanjeev commented Apr 1, 2022 • edited Loading

LakshSingla left a comment

Choose a reason for hiding this comment

adarshsanjeev commented Apr 26, 2022

LakshSingla left a comment

Choose a reason for hiding this comment

abhishekagarwal87 Apr 29, 2022

Choose a reason for hiding this comment

adarshsanjeev May 2, 2022

Choose a reason for hiding this comment

abhishekagarwal87 Apr 29, 2022

Choose a reason for hiding this comment

adarshsanjeev May 2, 2022

Choose a reason for hiding this comment

LakshSingla May 6, 2022

Choose a reason for hiding this comment

adarshsanjeev May 9, 2022

Choose a reason for hiding this comment

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

adarshsanjeev commented Apr 1, 2022 •

edited

Loading