-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
build(deps): upgrade sqlparser to 0.47.0 #10392
Conversation
Signed-off-by: tison <wander4096@gmail.com>
@jmhain Thanks for your patch. I've applied it to this branch. Let us continue fixing the breaking changes here when we have time :D |
2ed2daa
to
d9fd47f
Compare
Pushed a commit to handle other breaking changes. Now remains the JsonAccess and Agg func refactor. I'm still understanding how they are logically changed ... |
Signed-off-by: tison <wander4096@gmail.com>
d9fd47f
to
c112222
Compare
datafusion/sql/src/expr/mod.rs
Outdated
operator: JsonOperator::Colon, | ||
right, | ||
} => { | ||
SQLExpr::JsonAccess { value, path } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb @jmhain, do you have ideas on how we should modify this branch now?
Previously, we handled xxx::yyy
in this branch. But now, JsonOperator::Colon
no longer exists.
It seems to me that we should handle path.len() == 1
or path.len() == 2
and build a GetFieldAccess::ListRange
as the original logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed f29af9d to try it out.
The rest agg parts look much wider that I may hardly handle it by myself in a short time ..
Signed-off-by: tison <wander4096@gmail.com>
Seems that we should merge |
I am going to give this PR a look to see if I can help unstick it |
There is still a failure which I haven't had a chance to look into: cargo test --test sqllogictests -- array
...
Running "array_query.slt"
Running "array.slt"
External error: query failed: DataFusion error: SQL error: ParserError("Expected variant object key name, found: 2")
[SQL] select make_array(1, 2, 3)[1:2], make_array(1.0, 2.0, 3.0)[2:3], make_array('h', 'e', 'l', 'l', 'o')[2:4];
at test_files/array.slt:859
Error: Execution("1 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`
Caused by:
process didn't exit successfully: `/Users/andrewlamb/Software/datafusion/target/debug/deps/sqllogictests-b3a0a4f302be541c array` (exit status: 1)
|
@@ -2795,3 +2795,10 @@ SELECT '2000-12-01 04:04:12' AT TIME ZONE 'America/New York'; | |||
# abbreviated timezone is not supported | |||
statement error | |||
SELECT '2023-03-12 02:00:00' AT TIME ZONE 'EDT'; | |||
|
|||
|
|||
# Test current_time without parentheses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While updating the code I realized I almost broke this, so I added a new test (also propose adding it to main
here: #10509)
Since we're on the sqlparser-rs main branch now, I'd consider this patch as ready to merge. But of course it's better to have a new release to sqlparser-rs and we move to that version. Hopefully it contains what it has now without new breaking changes :P |
Window specific test failed. I don't have a windows env to debug ...
|
Thank you so much @tisonkun -- I agree I will go make a sqlparser release now without any new changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just released version 0.47.0 of sqlparser to crates.io: apache/datafusion-sqlparser-rs#1251 / https://crates.io/crates/sqlparser/0.47.0
} | ||
} | ||
/// RETURN or AS function body | ||
pub function_body: Option<Expr>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @milenkovicm -- sqlparser changed how the body was represented so now this changes as well to a generic Expr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know, @alamb
I just wonder if this change will diverge sqlparser from Postgres?
I guess function like this
CREATE FUNCTION strlen(name TEXT)
RETURNS int LANGUAGE plrust AS
$$
Ok(Some(name.unwrap().len() as i32))
$$;
Would not be possible without wrapping body with string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we may need to add $$
string parsing support in sqlparser. I'll try and file a ticket later todday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we should previously support $$
function body. If it fails to parse now, it's a regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to work, apache/datafusion-sqlparser-rs#1185
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test and made this syntax work in this pr in 8446851
Thanks @tisonkun -- I updated this PR to use the newly released 0.47.0 version of sqlparser-rs and updated the description. I suppose now we just need to debug the windows test (I have an idea) and we'll be good to go |
.github/workflows/rust.yml
Outdated
@@ -334,6 +334,8 @@ jobs: | |||
run: | | |||
export PATH=$PATH:$HOME/d/protoc/bin | |||
cargo test --lib --tests --bins --features avro,json,backtrace | |||
# 8MB stack size otherwise the cli_integration test fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to workaround the issue @tisonkun reports #10392 (comment)
If it passes CI I will file a ticket to track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems failure retains :<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 reverting the change and will try something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disabled the test and filed #10793 to track
This reverts commit b5743d5.
My plan for this PR is to disable the failing test on windows and file a ticket to investigate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a final merge up from main to make sure we don't have any logical conflicts and then I plan to merge this PR |
* build(deps): upgrade sqlparser to 0.46.0 Signed-off-by: tison <wander4096@gmail.com> * function and cast fixups * catchup refactors Signed-off-by: tison <wander4096@gmail.com> * try migrate json expr Signed-off-by: tison <wander4096@gmail.com> * Update for changes in sqlparser * Update dependencies * handle zero argument form * fmt * fixup more Signed-off-by: tison <wander4096@gmail.com> * fixup more Signed-off-by: tison <wander4096@gmail.com> * try use jmhain's branch Signed-off-by: tison <wander4096@gmail.com> * fix compile FunctionArgumentClause exhausted Signed-off-by: tison <wander4096@gmail.com> * fix compile set multi vars Signed-off-by: tison <wander4096@gmail.com> * fix compile new string values Signed-off-by: tison <wander4096@gmail.com> * fix compile set multi vars Signed-off-by: tison <wander4096@gmail.com> * fix compile Subscript Signed-off-by: tison <wander4096@gmail.com> * cargo fmt Signed-off-by: tison <wander4096@gmail.com> * revert workaround on values Signed-off-by: tison <wander4096@gmail.com> * Rework field access * update lock * fix doc * try catchup new sqlparser version Signed-off-by: tison <wander4096@gmail.com> * fixup timezone expr Signed-off-by: tison <wander4096@gmail.com> * fixup params Signed-off-by: tison <wander4096@gmail.com> * lock Signed-off-by: tison <wander4096@gmail.com> * Update to sqlparser 0.47.0 * Update rust stack size on windows * Revert "Update rust stack size on windows" This reverts commit b5743d5. * Add test + support for `$$` function definition * Disable failing windows CI test * fmt * simplify test * fmt --------- Signed-off-by: tison <wander4096@gmail.com> Co-authored-by: Joey Hain <hain.joseph@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Upgrade sqlparser to
0.46.00.47.0. This should handle a few of breaking changes:AggregateExpressionWithFilter
,ArraySubquery
,ListAgg
andArrayAgg
datafusion-sqlparser-rs#1247This is working in progress.
Blocked at apache/datafusion-sqlparser-rs#1283.