-
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
feat: customize column default values for external tables #8415
Conversation
@@ -232,11 +232,12 @@ async fn roundtrip_custom_listing_tables() -> Result<()> { | |||
WITH ORDER (c ASC) | |||
LOCATION '../core/tests/data/window_2.csv';"; | |||
|
|||
let plan = ctx.sql(query).await?.into_optimized_plan()?; |
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 always returned an EmptyRelation before, and did not achieve the testing purpose as expected.
use log::debug; | ||
use std::any::Any; | ||
use std::collections::HashMap; |
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 introduced hashbrown::HashMap
in #8283, now I am replacing it with std::collections::HashMap
.
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.
Thank you @jonahgao -- I think this PR looks great. I have one small suggestion related to an extra test but I also think we could merge without that change
|
||
let bytes = logical_plan_to_bytes(&plan)?; | ||
let logical_round_trip = logical_plan_from_bytes(&bytes, &ctx)?; | ||
assert_eq!(format!("{plan:?}"), format!("{logical_round_trip:?}")); | ||
// Use exact matching to verify everything, such as column defaults |
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.
Nice -- I think this check predated the LogicalPlan::PartialEq
implementation. I think this is a nice change
1 | ||
|
||
query IIIT rowsort | ||
select a,b,c,d from test_column_defaults |
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.
Could you also add a test that the value of now()
was set?
Maybe something basic like
-- Expect all rows to be true as now() was inserted into the table
select e < now() from test_column_defaults
?
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.
Good idea! Added.
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 again @jonahgao
---- | ||
1 | ||
|
||
# Ensure that the default expression `now()` is evaluated during insertion, not optimized away. |
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.
👌
* feat: customize column default values for external tables * fix test * tests from reviewing
Which issue does this PR close?
It is a continuation of #8283.
Rationale for this change
We previously implemented customizing column default values on memory tables, and this PR implements it for external tables.
What changes are included in this PR?
Support customizing column default values for external tables.
This feature will take effect when inserting.
Are these changes tested?
Yes
Are there any user-facing changes?
Yes.
Add a new field
column_defaults
inDdlStatement::CreateExternalTable
.