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

Misc cleanup of errors and docs #1720

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Misc cleanup of errors and docs #1720

wants to merge 4 commits into from

Conversation

alancai98
Copy link
Member

Relevant Issues

  • None

Description

  • Cleanup of AST DDL annotation
  • Copies over AST identifier comment over to SPI identifier
  • Fix error message string for / and %
  • Use a PartiQL divide-by-zero error rather than use Java's default arithmetic error for / and % of decimals + numerics
  • Fix strict mode behavior w/ missing literal to not error
  • Update partiql-tests submodule

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 requested a review from johnedquinn January 18, 2025 00:01
@alancai98 alancai98 self-assigned this Jan 18, 2025
@@ -38,7 +38,7 @@ public static class StructField extends AstNode {

private final boolean optional;

@Nullable
@NotNull
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) lists should be @NotNull rather than @Nullable. Taken from #1692

val dividendStr = prepare(dividend.toString(), " -- ", " / 0")
return "Division$dividendTypeStr by zero$dividendStr$dividendTypeStr."
val dividendStr = prepare(dividend.toString(), " ")
return "Cannot divide$dividendStr$dividendTypeStr by zero."
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) error message follows what's suggested from PError. This new error allows for reuse for / and %. Also the 0 may be a decimal or floating point zero.

@@ -15,7 +15,8 @@ public class Identifier private constructor(
) : Iterable<Identifier.Simple> {

/**
* Returns the unqualified name part.
* Returns the right-most simple identifier of the qualified identifier. For example, for an identifier
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) copied from AST's Identifier javadoc

Copy link

github-actions bot commented Jan 18, 2025

CROSS-ENGINE-REPORT ❌

BASE (LEGACY-V0.14.8) TARGET (EVAL-7457A13) +/-
% Passing 89.67% 95.00% 5.33% ✅
Passing 5287 5620 333 ✅
Failing 609 76 -533 ✅
Ignored 0 220 220 🔶
Total Tests 5896 5916 20 ✅

Testing Details

  • Base Commit: v0.14.8
  • Base Engine: LEGACY
  • Target Commit: 7457a13
  • Target Engine: EVAL

Result Details

  • ❌ REGRESSION DETECTED. See Now Failing/Ignored Tests. ❌
  • Passing in both: 2622
  • Failing in both: 17
  • Ignored in both: 0
  • PASSING in BASE but now FAILING in TARGET: 20
  • PASSING in BASE but now IGNORED in TARGET: 98
  • FAILING in BASE but now PASSING in TARGET: 180
  • IGNORED in BASE but now PASSING in TARGET: 0

Now FAILING Tests ❌

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

Now IGNORED Tests ❌

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

Now Passing Tests

180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-7457A13). 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.

@alancai98
Copy link
Member Author

alancai98 commented Jan 18, 2025

New conformance tests are unrelated to this PR's changes.

See this PR's conformance summary -- https://github.com/partiql/partiql-lang-kotlin/actions/runs/12838981007/attempts/1#summary-35805684691

See other PR's conformance summary which just updates partiql-tests (to the version in this PR) -- https://github.com/partiql/partiql-lang-kotlin/actions/runs/12838851749/attempts/1#summary-35805197280

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.

1 participant