Skip to content

Update Graph query support for SQL 2023 conformance #552

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

Merged
merged 9 commits into from
Apr 8, 2025

Conversation

jpschorr
Copy link
Contributor

@jpschorr jpschorr commented Mar 27, 2025

Overview

This PR updates

  • lexing
  • parsing
  • AST
  • pretty-printing
  • tests for the above

The changes are made to conform to GRAPH_TABLE query as described in SQL 2023. This is largely similar to the direction that was described by the GPML paper (https://arxiv.org/abs/2112.06217), but with some addtional SQL-isms and some extensions.

Evaluation of graph patterns are unchanged.

A large part of this PR are tests that assure roundtripping (text -> parse -> pretty-print -> parse) and pretty-print snapshot tests for the updated grammar.

SQL

If you are reviewing this PR, you may find the following useful:

  1. SQL:2023 Section 16

This PR's changes pass all existing experimental graph tests (and improved conformance over the GPML-based grammar). Only test name changes were needed due to some test name generation collisions.
Cf. partiql/partiql-tests#133


This PR builds on previous PRs for implementing GPML-style graph query.
Cf. #546, #547, #548, #549, #551, #553


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

Copy link

github-actions bot commented Mar 27, 2025

Conformance comparison report

Base (38cc1e5) 4b4c2aa +/-
% Passing 86.85% 87.54% 0.70%
✅ Passing 5731 5777 46
❌ Failing 868 822 -46
🔶 Ignored 0 0 0
Total Tests 6599 6599 0

Number passing in both: 5709

Number failing in both: 822

Number passing in Base (38cc1e5) but now fail: 0

Number failing in Base (38cc1e5) but now pass: 34

The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass:

Click here to see
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n2e0_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::strict_left_undirected_shorthand
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n2d1_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::permissive_left_right_undirected_shorthand
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n1e0_match_y
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n2e0_match_y
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n1u1_match_y
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n2u2_match_y
  • partiql_tests::eval::primitives::operators::operators::case_operator::case::strict_searched_case
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::strict_right_undirected_shorthand
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n1d2_match_y
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n0e0_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::strict_undirected_shorthand
  • partiql_tests::eval::primitives::operators::operators::case_operator::case::strict_simple_case_no_else
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n0e0_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::strict_left_right_undirected_shorthand
  • partiql_tests::eval::primitives::operators::operators::case_operator::case::strict_searched_case_no_else
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n2u1_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::permissive_right_shorthand
  • partiql_tests::eval::primitives::operators::operators::case_operator::case::strict_simple_case
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::permissive_left_undirected_shorthand
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n2d1_match_y
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n2u2_match_y
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n1u1_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::strict_left_shorthand
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::strict_left_right_shorthand
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::permissive_n2u1_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::permissive_left_shorthand
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::permissive_right_undirected_shorthand
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::permissive_undirected_shorthand
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::permissive_left_right_shorthand
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n1e0_match_y
  • partiql_tests::eval::experimental::graph::graph::directionality::directionality::strict_right_shorthand
  • partiql_tests::eval::experimental::graph::graph::small_graphs::small_graphs::strict_n1d2_match_y

Copy link

codecov bot commented Mar 27, 2025

Codecov Report

Attention: Patch coverage is 91.26467% with 67 lines in your changes missing coverage. Please review.

Project coverage is 82.16%. Comparing base (38cc1e5) to head (d4786df).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
partiql-logical-planner/src/graph.rs 55.31% 36 Missing and 6 partials ⚠️
partiql-ast/src/visit.rs 65.95% 16 Missing ⚠️
partiql-parser/src/lexer/partiql.rs 86.11% 5 Missing ⚠️
partiql-ast/src/pretty/graph.rs 99.35% 2 Missing and 1 partial ⚠️
...sion/partiql-extension-visualize/src/ast_to_dot.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #552      +/-   ##
==========================================
+ Coverage   81.93%   82.16%   +0.23%     
==========================================
  Files         108      109       +1     
  Lines       22721    23143     +422     
  Branches    22721    23143     +422     
==========================================
+ Hits        18616    19015     +399     
- Misses       3567     3594      +27     
+ Partials      538      534       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jpschorr jpschorr changed the title Feat sql graphtable Update Graph query support for SQL 2023 compatibility Apr 4, 2025
@jpschorr jpschorr force-pushed the feat-sql-graphtable branch from 1013a7f to ddc7b4c Compare April 4, 2025 22:21
@jpschorr jpschorr changed the base branch from feat-lower-gpml to feat-lower-gpml-merge April 4, 2025 23:12
@jpschorr jpschorr force-pushed the feat-sql-graphtable branch from 61455a8 to cfd306a Compare April 5, 2025 04:31
Base automatically changed from feat-lower-gpml-merge to main April 5, 2025 04:33
@jpschorr jpschorr force-pushed the feat-sql-graphtable branch from cfd306a to d3cb98d Compare April 5, 2025 04:39
@jpschorr jpschorr changed the base branch from main to chore-upgrade-deps April 5, 2025 05:16
@jpschorr jpschorr force-pushed the feat-sql-graphtable branch 2 times, most recently from bf2de54 to 1b08958 Compare April 5, 2025 16:29
@jpschorr jpschorr changed the title Update Graph query support for SQL 2023 compatibility Update Graph query support for SQL 2023 conformance Apr 7, 2025
@jpschorr jpschorr marked this pull request as ready for review April 7, 2025 19:02
@jpschorr jpschorr requested review from alancai98 and am357 April 7, 2025 19:02
Base automatically changed from chore-upgrade-deps to main April 7, 2025 19:07
@jpschorr jpschorr force-pushed the feat-sql-graphtable branch from f1b8f37 to 5d16ec0 Compare April 7, 2025 19:09
@jpschorr jpschorr force-pushed the feat-sql-graphtable branch from 5d16ec0 to d4786df Compare April 7, 2025 20:01
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

nice work implementing all the parser + ast implementations of SQL2023 graphs queries. just one minor comment

expression: doc
---
========================================================================================================================================================================================================
SELECT * FROM (g MATCH ( (x)-[e]->*(y) ) |+| (z) ~ (q) )
Copy link
Member

Choose a reason for hiding this comment

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

perhaps can call out somewhere that we choose to make the GRAPH_TABLE keyword optional? SQL 2023's BNF definition makes it required

<table primary> ::=
!! All alternatives from ISO/IEC 9075-2
| <graph table derived table>
<graph table derived table> ::=
<graph table> [ <correlation or recognition> ]
<graph table> ::=
GRAPH_TABLE
<left paren> <graph reference> MATCH <graph pattern> <graph table shape>
<right paren>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a result of treating MATCH 'just' like any other expression. E.g., PartiQL allows SELECT * FROM <anything that is a PartiQL expression here> and (<expr> MATCH <graph-pattern>) is defined as an expression.

@jpschorr jpschorr merged commit 6dd1cd3 into main Apr 8, 2025
19 checks passed
@jpschorr jpschorr deleted the feat-sql-graphtable branch April 8, 2025 18:24
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.

3 participants