refactor!: proof_of_sql_parser::intermediate_ast::OrderBy with sqlparser::ast::OrderByExpr in the proof-of-sql crate
#450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
varshith257
force-pushed
the
refactor-orderby
branch
from
fb9aa62 to
d7d1606
Compare
iajoiner
reviewed
Jan 6, 2025
Comment on lines
+128
to
+141
| fn owned_column_to_expr<S: Scalar>(col: &OwnedColumn<S>) -> Expr { | ||
| match col { | ||
| OwnedColumn::Boolean(_) => Expr::Identifier("BooleanColumn".into()), | ||
| OwnedColumn::TinyInt(_) => Expr::Identifier("TinyIntColumn".into()), | ||
| OwnedColumn::SmallInt(_) => Expr::Identifier("SmallIntColumn".into()), | ||
| OwnedColumn::Int(_) => Expr::Identifier("IntColumn".into()), | ||
| OwnedColumn::BigInt(_) => Expr::Identifier("BigIntColumn".into()), | ||
| OwnedColumn::VarChar(_) => Expr::Identifier("VarCharColumn".into()), | ||
| OwnedColumn::Int128(_) => Expr::Identifier("Int128Column".into()), | ||
| OwnedColumn::Decimal75(_, _, _) => Expr::Identifier("DecimalColumn".into()), | ||
| OwnedColumn::Scalar(_) => Expr::Identifier("ScalarColumn".into()), | ||
| OwnedColumn::TimestampTZ(_, _, _) => Expr::Identifier("TimestampColumn".into()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm this leads to duplicates, right?
There was a problem hiding this comment.
I ha ve implemented it for bidirectional as some places needed it. i will debug and fix if we doesn't needed it
iajoiner
reviewed
Jan 6, 2025
iajoiner
reviewed
Jan 6, 2025
| ( | ||
| col.clone(), | ||
| OrderByExpr { | ||
| expr: owned_column_to_expr(col), |
There was a problem hiding this comment.
OK we can't do this since expressions are not by default identifiers.
iajoiner
reviewed
Jan 6, 2025
| )) | ||
| |order_by| -> PostprocessingResult<(OwnedColumn<S>, OrderByExpr)> { | ||
| let identifier = match &order_by.expr { | ||
| Expr::Identifier(ident) => ident.clone(), |
There was a problem hiding this comment.
We can order by expressions such as c0 + c1, not identifiers.
There was a problem hiding this comment.
If expressions are needed I think we need some rework here and I will revisit later here once all the open PRs are get merged
2 tasks
varshith257
force-pushed
the
refactor-orderby
branch
3 times, most recently
from
4775126 to
bf47e4d
Compare
…parser::ast::OrderByExpr` in the proof-of-sql crate
iajoiner
force-pushed
the
refactor-orderby
branch
from
bf47e4d to
981cf73
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.
Please go through the following checklist
!is used if and only if at least one breaking change has been introduced.source scripts/run_ci_checks.sh.Rationale for this change
This PR addresses the need to replace the
proof_of_sql_parser::OrderBywith thesqlparser::ast::OrderByExprin theproof-of-sqlcrate as part of a larger transition toward integrating thesqlparser.This change is a subtask of issue #235, with the main goal of streamlining the repository by switching to the
sqlparsercrate and gradually replacing intermediary constructs likeproof_of_sql_parser::intermediate_astwithsqlparser::ast.What changes are included in this PR?
proof_of_sql_parser::OrderByhave been replaced withsqlparser::ast::OrderByExprOrderByhas been updated to maintain the original functionality, ensuring no changes to the logic or behavior.Are these changes tested?
Yes
Part of #235