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

jayzhan211 · 2024-05-18T06:19:20Z

Which issue does this PR close?

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 <[email protected]>

jayzhan211 · 2024-05-18T06:19:54Z

datafusion/core/tests/optimizer_integration.rs

@@ -233,3 +241,120 @@ impl TableSource for MyTableSource {
        self.schema.clone()
    }
 }
+
+#[test]
+fn test_nested_schema_nullability() {


Move tests that use functions to core/tests

jayzhan211 · 2024-05-18T06:20:44Z

datafusion/optimizer/src/optimize_projections/mod.rs

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

-    #[test]
-    fn test_struct_field_push_down() -> Result<()> {


Move to slt

Signed-off-by: jayzhan211 <[email protected]>

alamb · 2024-05-20T16:06:24Z

datafusion/core/tests/expr_api/mod.rs

@@ -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"),


alamb

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

alamb · 2024-05-20T16:11:54Z

datafusion/functions-array/src/expr_ext.rs

+// specific language governing permissions and limitations
+// under the License.
+
+//! Extension methods for Expr.


This is a very clever idea. Thank you @jayzhan211

alamb · 2024-05-20T16:12:34Z

datafusion/functions-array/src/expr_ext.rs

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


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?

alamb · 2024-05-20T16:13:10Z

datafusion/functions/src/core/expr_ext.rs

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


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

alamb · 2024-05-20T16:13:51Z

datafusion/physical-expr/src/planner.rs

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


Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-05-21T00:07:18Z

Thanks, @alamb !

…ith `FieldAccessor`, `IndexAccessor`, and `SliceAccessor` (apache#10568) * remove expr Signed-off-by: jayzhan211 <[email protected]> * add expr extension Signed-off-by: jayzhan211 <[email protected]> * doc Signed-off-by: jayzhan211 <[email protected]> * move test that has struct Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * add foc and fix displayed name Signed-off-by: jayzhan211 <[email protected]> * rm test Signed-off-by: jayzhan211 <[email protected]> * rebase Signed-off-by: jayzhan211 <[email protected]> * move doc Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]>

datafusion replaced Expr::GetIndexField with a FieldAccessor trait. Ref apache/datafusion#10568 Ref apache/datafusion#10769

* 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

…ith `FieldAccessor`, `IndexAccessor`, and `SliceAccessor` (apache#10568) * remove expr Signed-off-by: jayzhan211 <[email protected]> * add expr extension Signed-off-by: jayzhan211 <[email protected]> * doc Signed-off-by: jayzhan211 <[email protected]> * move test that has struct Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> * add foc and fix displayed name Signed-off-by: jayzhan211 <[email protected]> * rm test Signed-off-by: jayzhan211 <[email protected]> * rebase Signed-off-by: jayzhan211 <[email protected]> * move doc Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 added 5 commits May 18, 2024 10:56

remove expr

6c8abb4

Signed-off-by: jayzhan211 <[email protected]>

add expr extension

3aff84b

Signed-off-by: jayzhan211 <[email protected]>

doc

5bf1d82

Signed-off-by: jayzhan211 <[email protected]>

move test that has struct

db6b48a

Signed-off-by: jayzhan211 <[email protected]>

fmt

1a754b4

Signed-off-by: jayzhan211 <[email protected]>

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

jayzhan211 commented May 18, 2024

View reviewed changes

jayzhan211 added 2 commits May 18, 2024 14:37

add foc and fix displayed name

dc3bfd7

Signed-off-by: jayzhan211 <[email protected]>

rm test

136f3df

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 marked this pull request as ready for review May 18, 2024 07:19

jayzhan211 added 2 commits May 19, 2024 14:01

Merge remote-tracking branch 'upstream/main' into rm-GetIndexedField

860b11b

rebase

26cb3d0

Signed-off-by: jayzhan211 <[email protected]>

alamb added the api change Changes the API exposed to users of the crate label May 20, 2024

alamb reviewed May 20, 2024

View reviewed changes

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

alamb approved these changes May 20, 2024

View reviewed changes

alamb mentioned this pull request May 20, 2024

Minor: Fix name in ArrayFunctionRewriter, error not panic if Expr::GetStructField is planned #10564

Closed

move doc

f70e1e6

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 merged commit 3ebc31d into apache:main May 21, 2024
23 checks passed

appletreeisyellow mentioned this pull request May 21, 2024

WIP(iox-10891): patched df upgrade 202-05-13 influxdata/arrow-datafusion#20

Closed

lewiszlw mentioned this pull request Jun 3, 2024

Cleanup GetIndexedField #10769

Merged

abhiaagarwal mentioned this pull request Jun 9, 2024

chore: bump to datafusion 39, arrow 52, pyo3 0.21 delta-io/delta-rs#2581

Merged

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jun 13, 2024

feat: remove GetIndexField

b98378e

datafusion replaced Expr::GetIndexField with a FieldAccessor trait. Ref apache/datafusion#10568 Ref apache/datafusion#10769

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jayzhan211 commented May 18, 2024 •

edited

Loading

jayzhan211 May 18, 2024

jayzhan211 May 18, 2024

alamb May 20, 2024

alamb left a comment

alamb May 20, 2024

alamb May 20, 2024

alamb May 20, 2024

alamb May 20, 2024

jayzhan211 commented May 21, 2024

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

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

Conversation

jayzhan211 commented May 18, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jayzhan211 May 18, 2024

Choose a reason for hiding this comment

jayzhan211 May 18, 2024

Choose a reason for hiding this comment

alamb May 20, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb May 20, 2024

Choose a reason for hiding this comment

alamb May 20, 2024

Choose a reason for hiding this comment

alamb May 20, 2024

Choose a reason for hiding this comment

alamb May 20, 2024

Choose a reason for hiding this comment

jayzhan211 commented May 21, 2024

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

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

jayzhan211 commented May 18, 2024 •

edited

Loading