Skip to content

Comments

Surface a user friendly error when PARTITIONED BY is omitted#12246

Merged
abhishekagarwal87 merged 4 commits intoapache:masterfrom
LakshSingla:calcite-ext-insert-modify
Feb 11, 2022
Merged

Surface a user friendly error when PARTITIONED BY is omitted#12246
abhishekagarwal87 merged 4 commits intoapache:masterfrom
LakshSingla:calcite-ext-insert-modify

Conversation

@LakshSingla
Copy link
Contributor

@LakshSingla LakshSingla commented Feb 9, 2022 •
edited
Loading

Description

#12163 makes PARTITIONED BY a required clause in INSERT queries. While this is required, if a user accidentally omits the clause, it emits a JavaCC/Calcite error, since it's syntactically incorrect. The error message is cryptic. Since it's a custom clause, this PR aims to make the clause optional on the syntactic side, but move the validation to DruidSqlInsert where we can surface a friendlier error.

Older error message:
Was expecting one of: "PARTITIONED" ... "ORDER" ... "LIMIT" ... "OFFSET" ... "FETCH" ... "NATURAL" ... "JOIN" ... "INNER" ... "LEFT" ... "RIGHT" ... "FULL" ... "CROSS" ... "," ... "OUTER" ... "EXTEND" ... "(" ... "FOR" ... "MATCH_RECOGNIZE" ... "." ... "AS" ... <IDENTIFIER> ... <QUOTED_IDENTIFIER> ... <BACK_QUOTED_IDENTIFIER> ... <BRACKET_QUOTED_IDENTIFIER> ... <UNICODE_QUOTED_IDENTIFIER> ... "TABLESAMPLE" ... "WHERE" ... "GROUP" ... "HAVING" ... "WINDOW" ... "UNION" ... "INTERSECT" ... "EXCEPT" ... "MINUS"

Newer error message:
INSERT statements should specify PARTITIONED BY clause explicitly.

Also, to specify insert granularity as ALL, one can either mention ALL TIME (as in the original proposal) or ALL (after the change).

Key changed/added classes in this PR
  • insert.ftl
  • DruidSqlInsert

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

gianm
gianm reviewed Feb 9, 2022
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

The change looks good, just had a comment on the error message.

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java Outdated
);
Preconditions.checkNotNull(partitionedBy); // Shouldn't hit due to how the parser is written
if (partitionedBy == null) {
throw new ParseException("INSERT statements should specify PARTITIONED BY clause explictly");
Copy link
Contributor

@gianm gianm Feb 9, 2022

Choose a reason for hiding this comment

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

The word "must" is better than "should" here. (RFC 2119 doesn't govern here, but is still a helpful guide for technical writing. "Should" means "recommended, but not strictly required".)

Copy link
Contributor Author

@LakshSingla LakshSingla Feb 10, 2022

Choose a reason for hiding this comment

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

Updated the error message. Thanks for the tip.

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java Outdated
* While partitionedBy and partitionedByStringForUnparse can be null as arguments to the constructor, this is
* disallowed (semantically) and the constructor performs checks to ensure that. This helps in producing friendly
* errors when the PARTITIONED BY custom clause is not present, and keeps its error separate from JavaCC/Calcite's
* custom errors which can be cryptic when someone accidentaly forgets to explicitly specify the PARTITIONED BY clause
Copy link
Contributor

@gianm gianm Feb 9, 2022

Choose a reason for hiding this comment

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

accidentally (spelling)

@LakshSingla LakshSingla requested a review from gianm February 10, 2022 14:03
gianm
gianm approved these changes Feb 10, 2022
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM

@abhishekagarwal87 abhishekagarwal87 merged commit 5bd646e into apache:master Feb 11, 2022
@LakshSingla LakshSingla deleted the calcite-ext-insert-modify branch February 11, 2022 07:03
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gianm gianm gianm approved these changes

Assignees

No one assigned

Labels

Area - SQL

Projects

None yet

Milestone

0.23.0

Development

Successfully merging this pull request may close these issues.

4 participants

@LakshSingla @gianm @abhishekagarwal87 @clintropolis