Optimize struct and named_struct functions
#11688
Conversation
|
|
||
| #[test] | ||
| fn test_named_struct() { | ||
| // struct(1, 2.0, true) = { "foo": 1, "bar": 2.0, "baz": true } |
There was a problem hiding this comment.
Most of the test is already covered in datafusion/sqllogictest/test_files/struct.slt
There was a problem hiding this comment.
Okay, should the tests be removed then? I was following the lead of struct.rs, so should the test in that file be removed too for consistency?
There was a problem hiding this comment.
Sure! It is prefer to add test in slt, unless it is not possible or becomes much more complex
There was a problem hiding this comment.
Here are the instructions: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest
Ideally you should be able to extend one of the existing test files in https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest/test_files
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
…t_expr` caused by zipping then unzipping fields and values.
Rafferty97
force-pushed
the
optimize-named-struct-expr
branch
from
ebad5bf to
dcaa717
Compare
There was a problem hiding this comment.
Thank you @Rafferty97 -- this PR is looking good to me
This code looks nicer to me, but it is not clear that this would perform and better/worse than the existing code
Update: I see now the rationale on #11685 . I suspect the time required to create a few vecs will be hard to measure, but in any event I think the fact this code looks better makes it worth merging even if there is no performance change
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
To anyone following along, this is already covered in struct.slt
|
Thanks again @Rafferty97 |
3 participants
Which issue does this PR close?
Closes #11685.
Rationale for this change
Improve performance by eliminating unneeded heap allocations. See #11685 for details.
Are these changes tested?
Yes, new tests were added to cover the affected code.
Are there any user-facing changes?
No, it's only an implementation change.