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.

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

[v1] Remove DML and EXEC ast nodes #1552

Merged
merged 1 commit into from
Aug 27, 2024
Merged

[v1] Remove DML and EXEC ast nodes #1552

merged 1 commit into from
Aug 27, 2024

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented Aug 16, 2024

Relevant Issues

Description

Removes the DML and EXEC nodes from the partiql-ast.

Eventually we will want to add back the DML nodes that are spec'd out in the RFC and in SQL. The previous modeling in the partiql-ast may need revised since there were

  • some missing parts (e.g. DO UPDATE with assignment)
  • redundant components
  • TODOs throughout the parser and ast

So rather than committing to the potentially problematic AST modelings in the v1 release, I removed the nodes from the AST. We can always add those nodes back into the AST as they are needed.

Also worth noting that the previous ANTLR parser supported both the spec'd features as well as some legacy DML features that we don't want to support going forward. Removing the legacy DML features simplify the parsing rules and resolve some of the existing TODOs related to parsing DML.

EXEC is not fully spec'd out and was only added for a legacy customer. If it's ever needed after v1, we can add it back later.

Other Information

  • Updated Unreleased Section in CHANGELOG: [NO]

    • No, v1 branch
  • Any backward-incompatible changes? [YES]

    • Yes, but on v1 branch
  • Any new external dependencies? [NO]

  • Do your changes comply with the Contributing Guidelines
    and Code Style Guidelines? [YES]

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@alancai98 alancai98 changed the title V1 remove legacy DML and EXEC nodes [wip] [v1] remove legacy DML and EXEC nodes Aug 16, 2024
@alancai98 alancai98 changed the base branch from main to v1 August 16, 2024 22:13
@alancai98 alancai98 force-pushed the v1-remove-dml-exec-nodes branch from bfe6404 to c19931c Compare August 22, 2024 22:45
// DML
// ****************************************

@Test
Copy link
Member Author

Choose a reason for hiding this comment

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

(self-review) DML parsing tests were copied directly from PartiQLParserTest and runs on just the PIG parser.

// ****************************************
// EXEC clause parsing
// ****************************************
@Test
Copy link
Member Author

Choose a reason for hiding this comment

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

(self-review) EXEC parsing tests were copied directly from PartiQLParserTest and runs on just the PIG parser.

@alancai98 alancai98 changed the title [wip] [v1] remove legacy DML and EXEC nodes [v1] Remove DML and EXEC ast nodes Aug 22, 2024
Copy link

CROSS-ENGINE Conformance Report ❌

BASE (LEGACY-F6ADC8E) TARGET (EVAL-F6ADC8E) +/-
% Passing 90.45% 96.62% 6.17% ✅
Passing 5333 5697 364 ✅
Failing 563 199 -364 ✅
Ignored 0 0 0 ✅
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: f6adc8e
  • Base Engine: LEGACY
  • Target Commit: f6adc8e
  • Target Engine: EVAL

Result Details

  • ❌ REGRESSION DETECTED. See Now Failing Tests. ❌
  • Passing in both: 5205
  • Failing in both: 71
  • PASSING in BASE but now FAILING in TARGET: 128
  • FAILING in BASE but now PASSING in TARGET: 492

Now Failing Tests ❌

The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

Now Passing Tests

492 test(s) were previously failing in BASE (LEGACY-F6ADC8E) but now pass in TARGET (EVAL-F6ADC8E). Before merging, confirm they are intended to pass.

The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

CROSS-COMMIT-LEGACY Conformance Report ❌

BASE (LEGACY-4F783C7) TARGET (LEGACY-F6ADC8E) +/-
% Passing 90.47% 90.45% -0.02% ⭕
Passing 5334 5333 -1 ⭕
Failing 562 563 1 ⭕
Ignored 0 0 0 ✅
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: 4f783c7
  • Base Engine: LEGACY
  • Target Commit: f6adc8e
  • Target Engine: LEGACY

Result Details

  • ❌ REGRESSION DETECTED. See Now Failing Tests. ❌
  • Passing in both: 5333
  • Failing in both: 562
  • PASSING in BASE but now FAILING in TARGET: 1
  • FAILING in BASE but now PASSING in TARGET: 0

Now Failing Tests ❌

The following 1 test(s) were previously PASSING in BASE but are now FAILING in TARGET:

Click here to see
  1. MYSQL_SELECT_29, compileOption: LEGACY

CROSS-COMMIT-EVAL Conformance Report ✅

BASE (EVAL-4F783C7) TARGET (EVAL-F6ADC8E) +/-
% Passing 96.62% 96.62% 0.00% ✅
Passing 5697 5697 0 ✅
Failing 199 199 0 ✅
Ignored 0 0 0 ✅
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: 4f783c7
  • Base Engine: EVAL
  • Target Commit: f6adc8e
  • Target Engine: EVAL

Result Details

  • Passing in both: 5697
  • Failing in both: 199
  • PASSING in BASE but now FAILING in TARGET: 0
  • FAILING in BASE but now PASSING in TARGET: 0

@alancai98 alancai98 self-assigned this Aug 22, 2024
@alancai98 alancai98 marked this pull request as ready for review August 22, 2024 22:57
@alancai98 alancai98 requested a review from johnedquinn August 23, 2024 18:45
@alancai98 alancai98 merged commit 53098ee into v1 Aug 27, 2024
14 checks passed
@alancai98 alancai98 deleted the v1-remove-dml-exec-nodes branch August 27, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants