-
Notifications
You must be signed in to change notification settings - Fork 14
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
support arrow functions with ExprPlanner
#26
Conversation
tests/main.rs
Outdated
|
||
let expected = [ | ||
"+------------------+--------------------------------------+", | ||
"| name | json_get(test.json_data,Utf8(\"foo\")) |", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb it's worth noting that using UserDefinedSQLPlanner
means the default title for columns becomes the applied function rather than a pretty representation of the actual operator.
This differs from what we got from apache/datafusion#11137, see #22 where the column title for this was test.json_data -> Utf8(\"foo\")
I don't think this is a show stopper, just pointing it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One potential way workaround is to use Expr::alias
to name the expression something less unpleasing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I actually tried that unfortunately it got even more ugly when when you have a complex type and the column heading becomes something like alias(json_get(test.json_data,Utf8(\"foo\")), "json_data ->> "foo"") = "bar"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That certainly sounds worse -- I would expect the heading to only be "json_data ->> "foo"
(not alias(...)
) which is how it works for simple types
use alias
> select count(*), count(distinct stop_name) as foo, trip_tid from stops group by trip_tid limit 10;
+----------+-----+----------+
| count(*) | foo | trip_tid |
+----------+-----+----------+
| 18 | 0 | 54778923 |
| 17 | 0 | 54787869 |
| 1 | 0 | 54787875 |
| 5 | 2 | 54756517 |
| 6 | 2 | 54756510 |
| 19 | 0 | 54825475 |
| 18 | 0 | 54807326 |
| 27 | 0 | 54825423 |
| 20 | 0 | 54825510 |
| 20 | 0 | 54807384 |
+----------+-----+----------+
10 row(s) fetched.
Elapsed 0.043 seconds.
Non alias
> select count(*), count(distinct stop_name), trip_tid from stops group by trip_tid limit 10;
+----------+---------------------------------+----------+
| count(*) | count(DISTINCT stops.stop_name) | trip_tid |
+----------+---------------------------------+----------+
| 6 | 0 | 54804223 |
| 17 | 1 | 54804238 |
| 2 | 0 | 54804334 |
| 22 | 0 | 54779192 |
| 19 | 0 | 54825475 |
| 18 | 0 | 54807326 |
| 27 | 0 | 54825423 |
| 20 | 0 | 54825510 |
| 20 | 0 | 54807384 |
| 15 | 0 | 54825364 |
+----------+---------------------------------+----------+
10 row(s) fetched.
Elapsed 0.014 seconds.
Can you provide an example of what you mean by "complex types"? It sounds like it would be good bug / limitation to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've had another try and most of manually inserting aliases is working, see the penultimate commit below.
The only remain strange case is when doing (field->'foo')::int
, where my alias logic means that the cast is lost in the alias. See the tests test_arrow_cast_int
and test_arrow_double_nested_cast
.
I can't for the life of me work out how to make the alias (for test_arrow_cast_int
) (Utf8(\"{\"foo\": 42}\") -> Utf8(\"foo\"))::int
instead of just Utf8(\"{\"foo\": 42}\") -> Utf8(\"foo\")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ignore that message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apache/datafusion#11306 is causing issues.
UserDefinedSQLPlanner
ExprPlanner
Replaces #22
Uses apache/datafusion#11208.