fix(query): error on unsupported Cypher functions #114
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_tolower_works_in_simple_executor() { |
There was a problem hiding this comment.
@beinan This file is mainly for tests of datafusion-planner. For tests of the simple executor, maybe move them to another file?
There was a problem hiding this comment.
Done — moved the Simple executor test out of tests/test_datafusion_pipeline.rs into tests/test_simple_executor_pipeline.rs (and added a couple of small unit tests in simple_executor/expr.rs to cover the new branches). Thanks for the suggestion.
| VE::Variable(v) => col(v), | ||
| VE::Literal(v) => to_df_literal(v), | ||
| VE::Function { .. } | VE::Arithmetic { .. } => lit(0), | ||
| VE::Function { name, args } => match name.to_lowercase().as_str() { |
There was a problem hiding this comment.
Nice, I attempted this with Codex and I got a similar solution, but I was wondering if this was a sufficiently good solution, because tolower() and lower() are technically a subset of string scalar functions in Cypher, and there are potentially many more useful string functions that could be implemented in the future by other contributors.
Is there any particular reason why these would get special treatment in the simple executor? I also didn't fully understand why the planner would pick the simple executor in the case where this scalar Cypher function is called, bypassing the DataFusion planner entirely (where to_lowercase() is implemented. Curious if you had an explanation for that. Thanks!
There was a problem hiding this comment.
I also didn't fully understand why the planner would pick the simple executor in the case where this scalar Cypher function is called, bypassing the DataFusion planner entirely (where to_lowercase() is implemented
@prrao87 Are you referring to the issue #106?
I couldn't reproduce your error. The lower() example you provided works on my side. I guess the issue had already been fixed in the current codebase (but may not be in v0.4.0 if you directly used that)
There was a problem hiding this comment.
Ah yes, you're right. I am using the version 0.4.0 on PyPI, so that explains why it didn't work. Is there a plan to release 0.5.0 sometime soon with that and the WITH clause support? Thanks!
There was a problem hiding this comment.
Yes, it's a good time to release another version, maybe later this week or this weekend. Cc @beinan (in case you want a later date)
There was a problem hiding this comment.
Re: special-casing in the Simple executor — it’s a legacy/limited path that still gets used when callers explicitly choose (and historically it had the fallback). The goal here was to eliminate silent wrong results; adding was the smallest change to make common string-casing queries behave sensibly under Simple as well. Longer-term it probably makes sense to either (a) expand Simple’s function mapping in a more systematic way or (b) make Simple strictly reject anything beyond the minimal subset (so users are pushed to the DataFusion planner).
There was a problem hiding this comment.
good call, let's release 0.4.1 once this PR merged
There was a problem hiding this comment.
@beinan Thanks for the clarification. I would be inclined for the option (b) make Simple strictly reject anything beyond the minimal subset (so users are pushed to the DataFusion planner). In my opinion, the simple executor has performance benefits for some basic simple queries, such as property filtering. So a minimal subset of feature support would be good enough (complex queries should go to the datafusion planner)
There was a problem hiding this comment.
Agree — I think option (b) is the right long-term shape for ExecutionStrategy::Simple: keep it fast for the minimal subset, and proactively error on anything outside that subset to steer users to the DataFusion planner.
For this PR I kept the change scoped to eliminating silent wrong results (and added a tiny bit of string-casing support), but happy to follow up with tightening the Simple feature surface if you want.
beinan
force-pushed
the
fix/cypher-unsupported-functions-error
branch
from
705d693 to
dd2fc88
Compare
ChunxuTang
left a comment
ChunxuTang
left a comment
There was a problem hiding this comment.
@beinan LGTM. Just one minor comment for the UNWIND tests.
| // ============================================================================ | ||
|
|
||
| #[tokio::test] | ||
| async fn test_unwind_simple_list() { |
There was a problem hiding this comment.
Could you double-check whether you unintentionally removed the unwind tests? I guess it may be caused by merge conflicts.
There was a problem hiding this comment.
Good catch — you’re right, those tests got dropped during the rebase/conflict resolution. I restored the entire section from upstream into and pushed (commit ).
There was a problem hiding this comment.
Good catch — you’re right, those UNWIND tests got dropped during the rebase/conflict resolution.
I restored the entire // UNWIND Tests section from upstream into crates/lance-graph/tests/test_datafusion_pipeline.rs and pushed (commit 136dde8).
5 participants
Fixes #107.
0fallback)tolower/lower+toupper/upperin simple executorTests:
cargo test