Implement SQL validation based on grammar element #3039

ykmr1224 · 2024-09-17T22:14:49Z

Description

Implement SQL validation based on grammar element
This will reject query using deny list based on grammar element
Deny list can be defined per datasource type
This change validation applies only to async-query

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 · 2024-09-19T19:40:24Z

async-query-core/build.gradle

@@ -130,7 +130,7 @@ jacocoTestCoverageVerification {
            }
            limit {
                counter = 'BRANCH'
-                minimum = 1.0
+                minimum = 0.9


Reduced since there was a branch which could not be covered due to Spark SQL grammar.

Can we add it to the exclude list above instead of reducing this on entire module?

...c-query-core/src/main/java/org/opensearch/sql/spark/validator/SQLQueryValidationVisitor.java

async-query-core/src/main/java/org/opensearch/sql/spark/validator/FunctionType.java

...ery-core/src/main/java/org/opensearch/sql/spark/validator/S3GlueGrammarElementValidator.java

penghuo · 2024-09-19T21:55:31Z

...ery-core/src/main/java/org/opensearch/sql/spark/validator/S3GlueGrammarElementValidator.java

+              HINTS,
+              INLINE_TABLE,
+              FILE,
+              CROSS_JOIN,


disable Join? why?

It is because those joins are expensive.

PPL add JOIN support with ALL join types. https://github.com/opensearch-project/opensearch-spark/blob/main/docs/PPL-Join-command.md

We should align PPL and SQL capability.

Signed-off-by: Tomoyuki Morita <[email protected]>

codecov · 2024-09-20T21:17:01Z

Codecov Report

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

Project coverage is 94.43%. Comparing base (37188bd) to head (683126e).

Files with missing lines	Patch %	Lines
...sql/spark/validator/SQLQueryValidationVisitor.java	99.46%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3039      +/-   ##
============================================
+ Coverage     94.33%   94.43%   +0.10%     
- Complexity     5278     5387     +109     
============================================
  Files           519      528       +9     
  Lines         15002    15305     +303     
  Branches       1005     1010       +5     
============================================
+ Hits          14152    14454     +302     
  Misses          804      804              
- Partials         46       47       +1

Flag	Coverage Δ
sql-engine	`94.43% <99.69%> (+0.10%)`	⬆️

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.

ykmr1224 · 2024-09-20T22:58:06Z

Filed a issue for update/test spark version: #3041

Signed-off-by: Tomoyuki Morita <[email protected]>

penghuo

Thx!

dai-chen · 2024-09-23T18:03:01Z

async-query-core/build.gradle

@@ -130,7 +130,7 @@ jacocoTestCoverageVerification {
            }
            limit {
                counter = 'BRANCH'
-                minimum = 1.0
+                minimum = 0.9


Can we add it to the exclude list above instead of reducing this on entire module?

...ery-core/src/main/java/org/opensearch/sql/spark/validator/S3GlueGrammarElementValidator.java

Signed-off-by: Tomoyuki Morita <[email protected]>

* Implement SQL validation based on grammar element Signed-off-by: Tomoyuki Morita <[email protected]> * Add function types Signed-off-by: Tomoyuki Morita <[email protected]> * fix style Signed-off-by: Tomoyuki Morita <[email protected]> * Add security lake Signed-off-by: Tomoyuki Morita <[email protected]> * Add File support Signed-off-by: Tomoyuki Morita <[email protected]> * Integrate into SparkQueryDispatcher Signed-off-by: Tomoyuki Morita <[email protected]> * Fix style Signed-off-by: Tomoyuki Morita <[email protected]> * Add tests Signed-off-by: Tomoyuki Morita <[email protected]> * Integration Signed-off-by: Tomoyuki Morita <[email protected]> * Add comments Signed-off-by: Tomoyuki Morita <[email protected]> * Address comments Signed-off-by: Tomoyuki Morita <[email protected]> * Allow join types for now Signed-off-by: Tomoyuki Morita <[email protected]> * Fix style Signed-off-by: Tomoyuki Morita <[email protected]> * Fix coverage check Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]> (cherry picked from commit a87893a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

LantaoJin · 2024-09-25T00:37:15Z

This PR reminds me of another PR #2781 which is an implementation the Aggregate validation based on grammar. I haven't go through the whole details of this PR. Does this PR could resolve the problems described in #2781 (comment)? @ykmr1224 @penghuo

* Implement SQL validation based on grammar element * Add function types * fix style * Add security lake * Add File support * Integrate into SparkQueryDispatcher * Fix style * Add tests * Integration * Add comments * Address comments * Allow join types for now * Fix style * Fix coverage check --------- (cherry picked from commit a87893a) Signed-off-by: Tomoyuki Morita <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

ykmr1224 · 2024-09-26T04:00:43Z

This PR reminds me of another PR #2781 which is an implementation the Aggregate validation based on grammar. I haven't go through the whole details of this PR. Does this PR could resolve the problems described in #2781 (comment)? @ykmr1224 @penghuo

No, I think #2781 is for query executed in SQL plugin, but this PR is for async-query. And it is rather prohibiting grammar elements available in Spark SQL.

ykmr1224 marked this pull request as ready for review September 19, 2024 17:25

ykmr1224 requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vmmusings, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG and LantaoJin as code owners September 19, 2024 17:25

ykmr1224 added 10 commits September 19, 2024 11:49

Implement SQL validation based on grammar element

562113b

Signed-off-by: Tomoyuki Morita <[email protected]>

Add function types

d77e408

Signed-off-by: Tomoyuki Morita <[email protected]>

fix style

c9f960f

Signed-off-by: Tomoyuki Morita <[email protected]>

Add security lake

9665a62

Signed-off-by: Tomoyuki Morita <[email protected]>

Add File support

9fafcb9

Signed-off-by: Tomoyuki Morita <[email protected]>

Integrate into SparkQueryDispatcher

424be12

Signed-off-by: Tomoyuki Morita <[email protected]>

Fix style

7dde72a

Signed-off-by: Tomoyuki Morita <[email protected]>

Add tests

71cfa18

Signed-off-by: Tomoyuki Morita <[email protected]>

Integration

57a5289

Signed-off-by: Tomoyuki Morita <[email protected]>

Add comments

11ca4ff

Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 force-pushed the limit-sql-grammar branch from c44d89f to 11ca4ff Compare September 19, 2024 18:50

ykmr1224 commented Sep 19, 2024

View reviewed changes

penghuo reviewed Sep 19, 2024

View reviewed changes

ykmr1224 added 2 commits September 20, 2024 09:27

Address comments

d31bdc8

Signed-off-by: Tomoyuki Morita <[email protected]>

Allow join types for now

683126e

Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 mentioned this pull request Sep 20, 2024

[FEATURE] Come up with a way to update/test grammar validation when updating spark version #3041

Open

Fix style

4e8a02c

Signed-off-by: Tomoyuki Morita <[email protected]>

penghuo previously approved these changes Sep 23, 2024

View reviewed changes

dai-chen previously approved these changes Sep 23, 2024

View reviewed changes

Fix coverage check

b0e545e

Signed-off-by: Tomoyuki Morita <[email protected]>

ykmr1224 dismissed stale reviews from dai-chen and penghuo via b0e545e September 23, 2024 20:30

dai-chen approved these changes Sep 23, 2024

View reviewed changes

penghuo approved these changes Sep 23, 2024

View reviewed changes

ykmr1224 merged commit a87893a into opensearch-project:main Sep 23, 2024
12 of 14 checks passed

ykmr1224 added feature backport 2.x labels Sep 23, 2024

opensearch-trigger-bot bot mentioned this pull request Sep 23, 2024

[Backport 2.x] Implement SQL validation based on grammar element #3044

Merged

ykmr1224 mentioned this pull request Oct 4, 2024

Disable join types in validators #3056

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SQL validation based on grammar element #3039

Implement SQL validation based on grammar element #3039

ykmr1224 commented Sep 17, 2024 •

edited

Loading

ykmr1224 Sep 19, 2024

dai-chen Sep 23, 2024

penghuo Sep 19, 2024

ykmr1224 Sep 20, 2024

penghuo Sep 20, 2024

codecov bot commented Sep 20, 2024

ykmr1224 commented Sep 20, 2024

penghuo left a comment

dai-chen Sep 23, 2024

LantaoJin commented Sep 25, 2024

ykmr1224 commented Sep 26, 2024

Implement SQL validation based on grammar element #3039

Implement SQL validation based on grammar element #3039

Conversation

ykmr1224 commented Sep 17, 2024 • edited Loading

Description

Related Issues

Check List

ykmr1224 Sep 19, 2024

Choose a reason for hiding this comment

dai-chen Sep 23, 2024

Choose a reason for hiding this comment

penghuo Sep 19, 2024

Choose a reason for hiding this comment

ykmr1224 Sep 20, 2024

Choose a reason for hiding this comment

penghuo Sep 20, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2024

Codecov Report

ykmr1224 commented Sep 20, 2024

penghuo left a comment

Choose a reason for hiding this comment

dai-chen Sep 23, 2024

Choose a reason for hiding this comment

LantaoJin commented Sep 25, 2024

ykmr1224 commented Sep 26, 2024

ykmr1224 commented Sep 17, 2024 •

edited

Loading