Skip to content
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

Remove Expr::GetIndexedField, replace Expr::{field,index,range} with FieldAccessor, IndexAccessor, and SliceAccessor #10568

Merged
merged 10 commits into from
May 21, 2024

Conversation

jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented May 18, 2024

Which issue does this PR close?

Closes #10374.
Closes #10565

Rationale for this change

What changes are included in this PR?

After the change, you need to import

use datafusion_functions::core::expr_ext::FieldAccessor;
use datafusion_functions_array::expr_ext::{IndexAccessor, SliceAccessor};

to use old expression api.

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels May 18, 2024
@@ -233,3 +241,120 @@ impl TableSource for MyTableSource {
self.schema.clone()
}
}

#[test]
fn test_nested_schema_nullability() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move tests that use functions to core/tests

@@ -1037,29 +1037,6 @@ mod tests {
assert_optimized_plan_equal(plan, expected)
}

#[test]
fn test_struct_field_push_down() -> Result<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move to slt

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 marked this pull request as ready for review May 18, 2024 07:19
@alamb alamb added the api change Changes the API exposed to users of the crate label May 20, 2024
@@ -61,7 +63,7 @@ fn test_eq_with_coercion() {
#[test]
fn test_get_field() {
evaluate_expr_test(
get_field(col("props"), "a"),
col("props").field("a"),
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@alamb alamb changed the title Remove Expr::GetIndexedField and fix panic of field, index and range Remove Expr::GetIndexedField, replace Expr::{field,index,range} with FieldAccessor, IndexAccessor, and SliceAccessor May 20, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This is great -- thank you @jayzhan211 . I had one small comment regarding the placement of docstrings but I also think we could do that as a follow on PR

// specific language governing permissions and limitations
// under the License.

//! Extension methods for Expr.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very clever idea. Thank you @jayzhan211

}

impl IndexAccessor for Expr {
/// Return access to the element field. Example `expr["name"]`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the way that rustdoc works, this documentation won't appear on index -- can you please move the docs and example to IndexAccessor itself?

}

impl FieldAccessor for Expr {
/// Return access to the named field. Example `expr["name"]`
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise here I think we need to move the docs / example to FieldAccessor rather than on the impl in Expr

@@ -287,24 +284,6 @@ pub fn create_physical_expr(
input_dfschema,
execution_props,
)?),
Expr::GetIndexedField(GetIndexedField { expr: _, field }) => match field {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
Copy link
Contributor Author

Thanks, @alamb !

@jayzhan211 jayzhan211 merged commit 3ebc31d into apache:main May 21, 2024
23 checks passed
appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request May 21, 2024
…ith `FieldAccessor`, `IndexAccessor`, and `SliceAccessor` (apache#10568)

* remove expr

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* add expr extension

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* doc

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* move test that has struct

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* fmt

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* add foc and fix displayed name

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* rm test

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* rebase

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* move doc

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

---------

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jun 13, 2024
datafusion replaced Expr::GetIndexField with a FieldAccessor trait.

Ref apache/datafusion#10568
Ref apache/datafusion#10769
andygrove pushed a commit to apache/datafusion-python that referenced this pull request Jun 14, 2024
* deps: update datafusion to 39.0.0, pyo3 to 0.21, and object_store to 0.10.1

`datafusion-common` also depends on `pyo3`, so they need to be upgraded together.

* feat: remove GetIndexField

datafusion replaced Expr::GetIndexField with a FieldAccessor trait.

Ref apache/datafusion#10568
Ref apache/datafusion#10769

* feat: update ScalarFunction

The field `func_name` was changed to `func` as part of removing `ScalarFunctionDefinition` upstream.

Ref apache/datafusion#10325

* feat: incorporate upstream array_slice fixes

Fixes #670

* update ExectionPlan::children impl for DatasetExec

Ref apache/datafusion#10543

* update value_interval_daytime

Ref apache/arrow-rs#5769

* update regexp_replace and regexp_match

Fixes #677

* add gil-refs feature to pyo3

This silences pyo3's deprecation warnings for its new Bounds api.

It's the 1st step of the migration, and should be removed before merge.

Ref https://pyo3.rs/v0.21.0/migration#from-020-to-021

* fix signature for octet_length

Ref apache/datafusion#10726

* update signature for covar_samp

AggregateUDF expressions now have a builder API design, which removes arguments like filter and order_by

Ref apache/datafusion#10545
Ref apache/datafusion#10492

* convert covar_pop to expr_fn api

Ref: https://github.com/apache/datafusion/pull/10418/files

* convert median to expr_fn api

Ref apache/datafusion#10644

* convert variance sample to UDF

Ref apache/datafusion#10667

* convert first_value and last_value to UDFs

Ref apache/datafusion#10648

* checkpointing with a few todos to fix remaining compile errors

* impl PyExpr::python_value for IntervalDayTime and IntervalMonthDayNano

* convert sum aggregate function to UDF

* remove unnecessary clone on double reference

* apply cargo fmt

* remove duplicate allow-dead-code annotation

* update tpch examples for new pyarrow interval

Fixes #665

* marked q11 tpch example as expected fail

Ref #730

* add default stride of None back to array_slice
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
…ith `FieldAccessor`, `IndexAccessor`, and `SliceAccessor` (apache#10568)

* remove expr

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* add expr extension

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* doc

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* move test that has struct

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* fmt

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* add foc and fix displayed name

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* rm test

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* rebase

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* move doc

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

---------

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
2 participants