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

Issue using arrow_cast in ORDER BY expressions #9143

Closed
universalmind303 opened this issue Feb 6, 2024 · 9 comments · Fixed by #9610
Closed

Issue using arrow_cast in ORDER BY expressions #9143

universalmind303 opened this issue Feb 6, 2024 · 9 comments · Fixed by #9610
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@universalmind303
Copy link
Contributor

Describe the bug

originally opened in GlareDB/glaredb#2597

It seems that arrow_cast's special handling makes it so that it errors out when using it in an ORDER BY expr

To Reproduce

statement ok
create table ts (a int);

statement ok
insert into ts values (915148800);

# this errors
statement ok 
SELECT
  DATE_TRUNC('minute', arrow_cast(a, 'Timestamp(Second, None)')) AS minute
FROM
  ts
ORDER BY
  DATE_TRUNC('minute', arrow_cast(a, 'Timestamp(Second, None)'));

Expected behavior

It should be functionally equivalent in this example to using to_timestamp_seconds

SELECT
  DATE_TRUNC('minute', to_timestamp_seconds(a)) AS minute
FROM
  ts
ORDER BY
  DATE_TRUNC('minute', to_timestamp_seconds(a));

Additional context

No response

@universalmind303 universalmind303 added the bug Something isn't working label Feb 6, 2024
@brayanjuls
Copy link
Contributor

I would like to work on this one.

@viirya
Copy link
Member

viirya commented Feb 6, 2024

I actually began something with this fix, not finish it yet. Let me know if you are really interested in this. I can hold the fix.

@brayanjuls
Copy link
Contributor

I wanted to work on it as an exercise to also learn the internals, if there is a hurry to solve it then go for it, if not then I would be glad to work on this.

@viirya
Copy link
Member

viirya commented Feb 6, 2024

Okay, I think this is not urgent. I will hold the fix. If you don't have time to work this anymore later, please let me know. I will continue on this. Thank you.

@alamb
Copy link
Contributor

alamb commented Feb 7, 2024

Note another approach might be to help #8985 (which would allow us to remove the special case handling of arrow_cast and make it a normal UDF)

@brayanjuls
Copy link
Contributor

@alamb suggested a solution in that PR but I am still trying to understand where the arrow_cast UDF should be implemented after that PR is merged.

@alamb
Copy link
Contributor

alamb commented Feb 9, 2024

@alamb suggested a solution in that PR but I am still trying to understand where the arrow_cast UDF should be implemented after that PR is merged.

We are talking about the organization on #9100 -- I am sorry it is not yet fully finalized or written down @brayanjuls

I would propose putting it in datafusion-functions/src/core. However, there is not yet an example of doing so, but I can prioritize doing it so you have an example to follow (or you can take a shot at it on your own, but there is a bit of macro magic that is necessary)

@brayanjuls
Copy link
Contributor

I would propose putting it in datafusion-functions/src/core. However, there is not yet an example of doing so, but I can prioritize doing it so you have an example to follow (or you can take a shot at it on your own, but there is a bit of macro magic that is necessary)

I would prefer waiting for the examples as I have little experience creating rust declarative macros.

@alamb
Copy link
Contributor

alamb commented Feb 13, 2024

I would prefer waiting for the examples as I have little experience creating rust declarative macros.

Makes sense -- I will try and get a PR shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
4 participants