-
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: support the ergonomics of getting list slice with stride #8946
Conversation
sqlparser-rs does not support these syntax.
|
This is not valid in duckdb too, try |
Hi @Weijun-H -- I wonder if you know of another database that offers this method of accessing lists? |
I only know DuckDB: https://duckdb.org/docs/sql/functions/nested.html#slicing @alamb |
35013fb
to
d692128
Compare
This Is the problem of |
} | ||
|
||
/// Create a new [`GetIndexedFieldExpr`] for accessing the stride | ||
pub fn new_stride( |
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 is unused in this PR, and I think we might not need it too, since new_range
is only used in test, which may be removed in the future since we can test them in slt.
@@ -208,10 +208,15 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> { | |||
let key = create_physical_name(key, false)?; | |||
format!("{expr}[{key}]") | |||
} | |||
GetFieldAccess::ListRange { start, stop } => { | |||
GetFieldAccess::ListStride { |
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.
One small thing, I think stride is equivalent to step, IMO ListRange is more appropriate than ListStride
datafusion/sql/src/expr/mod.rs
Outdated
)?); | ||
let stop = Box::new(self.sql_expr_to_logical_expr( | ||
*right, | ||
// the last value could represent stop or stride |
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.
nit:
let (start, stop, stride) = if let SQLExpr::JsonAccess { left: l, operator: JsonOperator::Colon, right: r } = *left {
let start = Box::new(self.sql_expr_to_logical_expr(*l, schema, planner_context)?);
let stop = Box::new(self.sql_expr_to_logical_expr(*r, schema, planner_context)?);
let stride = Box::new(self.sql_expr_to_logical_expr(*right, schema, planner_context)?);
(start, stop, stride)
} else {
let start = Box::new(self.sql_expr_to_logical_expr(*left, schema, planner_context)?);
let stop = Box::new(self.sql_expr_to_logical_expr(*right, schema, planner_context)?);
let stride = Box::new(Expr::Literal(ScalarValue::Int64(Some(1))));
(start, stop, stride)
};
GetFieldAccess {
start,
stop,
stride
}
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.
LGTM, left some minor comments
83dca08
to
1ec7d43
Compare
7d7486b
to
c344374
Compare
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 @Weijun-H and @jayzhan211 🚀
@@ -750,6 +751,8 @@ fn roundtrip_get_indexed_field_list_range() -> Result<()> { | |||
GetFieldAccessExpr::ListRange { | |||
start: col_start, | |||
stop: col_stop, | |||
stride: Arc::new(Literal::new(ScalarValue::Int64(Some(1)))) |
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 think the clippy failure https://github.com/apache/arrow-datafusion/actions/runs/7690817195/job/20955254027 |
Which issue does this PR close?
Closes #8830
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?