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

Using Expr::field panics #10565

Closed
alamb opened this issue May 17, 2024 · 3 comments · Fixed by #10568
Closed

Using Expr::field panics #10565

alamb opened this issue May 17, 2024 · 3 comments · Fixed by #10568
Assignees
Labels
bug Something isn't working

Comments

@alamb
Copy link
Contributor

alamb commented May 17, 2024

Describe the bug

After #10375 merged Expr::field now panics if you use it (as we did in influxdb) DataFusion panics when you try to execute it

To Reproduce

Try to evaluate an expression like col("props").field("a")

Here is a full reproducer in the sql_integration test:

(venv) andrewlamb@Andrews-MacBook-Pro:~/Software/datafusion$ git diff
diff --git a/datafusion/core/tests/expr_api/mod.rs b/datafusion/core/tests/expr_api/mod.rs
index d7e839824..d4141a836 100644
--- a/datafusion/core/tests/expr_api/mod.rs
+++ b/datafusion/core/tests/expr_api/mod.rs
@@ -58,6 +58,25 @@ fn test_eq_with_coercion() {
     );
 }

+
+#[test]
+fn test_expr_field() {
+    // currently panics with
+    // Internal("NamedStructField should be rewritten in OperatorToFunction")
+    evaluate_expr_test(
+        col("props").field("a"),
+        vec![
+            "+------------+",
+            "| expr       |",
+            "+------------+",
+            "| 2021-02-01 |",
+            "| 2021-02-02 |",
+            "| 2021-02-03 |",
+            "+------------+",
+        ],
+    );
+}
+

Expected behavior

Ideally the test should pass Expr::field would continue to work

We could also potentially remove Expr::field but I think that would be less user friendly

Additional context

I am pretty sure I Expr::field is widely used so I think we should continue to support it if possible

@alamb
Copy link
Contributor Author

alamb commented May 17, 2024

I wonder if we could have Expr::field call get_field if the core functions feature was enabled and panic otherwise 🤔

That would be easy to use for most people and backwards compatible

@jayzhan211 jayzhan211 self-assigned this May 18, 2024
@jayzhan211
Copy link
Contributor

I can fix this along with #10374

@alamb
Copy link
Contributor Author

alamb commented May 20, 2024

Thank you @jayzhan211 🙏 -- I will review it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants