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.

Sign up for GitHub

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

Jump to bottom

Remove hard-coded aggregations from parser and ast #1464

Merged
merged 2 commits into from
May 30, 2024
Merged

Remove hard-coded aggregations from parser and ast #1464

merged 2 commits into from
May 30, 2024

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented May 15, 2024 •
edited
Loading

Relevant Issues

Description

  • Removes the hard-coded ANTLR parse rule for aggregation functions
  • Removes the dedicated aggregation node from AST. Set quantifier added to function call node.
  • Preserves legacy PIG parsing and AST behavior
  • Slight differences in modeling of COUNT(*)
    • Previously, would map to the function named COUNT_STAR with no args; changed to map to COUNT with no args

Other Information

  • Updated Unreleased Section in CHANGELOG: [NO]

    • No, v1 branch

  • Any backward-incompatible changes? [YES/NO]

    • Yes, AST modeling changes -- AST's call now represents both scalar and aggregate functions

  • Any new external dependencies? [NO]

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

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 self-assigned this May 15, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 16, 2024 •
edited
Loading

Conformance comparison report-Cross Engine

Base (eval) legacy +/-
% Passing 91.15% 92.51% 1.36%
✅ Passing 5304 5382 78
❌ Failing 515 436 -79
🔶 Ignored 0 0 0
Total Tests 5819 5818 -1
Number passing in both: 5071

Number failing in both: 203

Number passing in eval engine but fail in legacy engine: 233

Number failing in eval engine but pass in legacy engine: 312
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️
The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.
312 test(s) were failing in eval but now pass in legacy. 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.

Conformance comparison report-Cross Commit-EVAL

Base (4f89c2d) 7663edc +/-
% Passing 90.70% 91.15% 0.45%
✅ Passing 5278 5304 26
❌ Failing 541 515 -26
🔶 Ignored 0 0 0
Total Tests 5819 5819 0
Number passing in both: 5278

Number failing in both: 515

Number passing in Base (4f89c2d) but now fail: 1

Number failing in Base (4f89c2d) but now pass: 27
⁉️ CONFORMANCE REPORT REGRESSION DETECTED ⁉️. The following test(s) were previously passing but now fail:

Click here to see
  • Example 6 — Value Coercion, compileOption: LEGACY
27 test(s) were previously failing but now pass. 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.

Conformance comparison report-Cross Commit-LEGACY

Base (4f89c2d) 7663edc +/-
% Passing 92.51% 92.51% 0.00%
✅ Passing 5382 5382 0
❌ Failing 436 436 0
🔶 Ignored 0 0 0
Total Tests 5818 5818 0
Number passing in both: 5382

Number failing in both: 436

Number passing in Base (4f89c2d) but now fail: 0

Number failing in Base (4f89c2d) but now pass: 0

@alancai98 alancai98 marked this pull request as draft May 16, 2024 00:19
alancai98
alancai98 commented May 16, 2024
View reviewed changes
partiql-spi/src/main/kotlin/org/partiql/spi/connector/sql/builtins/AggCountStar.kt
@@ -1,25 +0,0 @@
// ktlint-disable filename
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) COUNT(*) builtin function deleted in favor of mapping COUNT(*) (COUNT() in ast) to COUNT(1)

partiql-spi/src/main/kotlin/org/partiql/spi/connector/sql/builtins/AggCount.kt Show resolved Hide resolved
partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt
return@build callAgg(strategy, name, arg, metas)
}

private fun String.isAggregateCall(): Boolean {
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) in the legacy PIG AST and conversion from partiql-ast to PIG AST, keep the same behavior as before with respect to supported aggregations. E.g.

  • SUM(*) and F(*) will still error
  • Aggregations require exactly 1 argument

Hard-coding the aggregations in the conversion to PIG AST since this is legacy code that will be removed by the v1 release.

partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RelConverter.kt
}
}

private fun String.isAggregateCall(): Boolean {
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) since the function resolution specification for scalar vs aggregate functions has not yet been defined, opt to hard-code the aggregation logic to the planner for now. Once we formalize the function resolution, we can remove this hard-coded logic.

partiql-ast/src/test/kotlin/org/partiql/ast/sql/SqlDialectTest.kt
function = id("FOO")
setq = SetQuantifier.DISTINCT
args += exprVar(id("x"), Expr.Var.Scope.DEFAULT)
args += exprVar(id("y"), Expr.Var.Scope.DEFAULT)
}
},
expect("COUNT(*)") {
exprAgg {
function = id("COUNT_STAR")
exprCall {
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) modeling difference in partiql-ast for COUNT(*). Previously it was an exprAgg with a function name of COUNT_STAR and no arguments. Changed to just be an exprCall with function name of COUNT and no arguments, which I think is slightly cleaner.

This is how Trino opts to represent COUNT(*) in their AST -- https://github.com/trinodb/trino/blob/429f8a8257d38e09b11a69571456c346f973373e/core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java#L436-L438.

@alancai98 alancai98 marked this pull request as ready for review May 18, 2024 00:37
@alancai98
Copy link
Member Author

PR should be ready for review. Regarding future work related to user-defined aggregations, function resolution between scalars and aggregations, and qualified paths for aggregations, I'll need to discuss further w/ the PartiQL team on these topics when I'm back online at the end of the month.

RCHowell
RCHowell requested changes May 21, 2024
View reviewed changes
Copy link
Member

@RCHowell RCHowell left a comment

Choose a reason for hiding this comment

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

Perfect! Very clean and I'm happy to see this item tackled.

partiql-ast/src/main/kotlin/org/partiql/ast/helpers/ToLegacyAst.kt Outdated Show resolved Hide resolved
partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RelConverter.kt Outdated Show resolved Hide resolved
@alancai98 alancai98 requested a review from RCHowell May 29, 2024 18:20
@RCHowell RCHowell merged commit 22bb461 into v1 May 30, 2024
14 checks passed
@RCHowell RCHowell deleted the remove-agg-from-ast branch May 30, 2024 18:54
@alancai98 alancai98 mentioned this pull request May 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RCHowell RCHowell RCHowell approved these changes

Assignees

@alancai98 alancai98

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alancai98 @RCHowell