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

Patched DataFusion, Oct 15, 2024 #47

Closed
wants to merge 12 commits into from

Conversation

wiedld
Copy link
Collaborator

@wiedld wiedld commented Dec 12, 2024

This takes us up to Oct-15 SHA 399e840.

It advances DF by adding these oct-7 to oct-15 commits: 577e4bb...influxdata:arrow-datafusion:399e8403d2fb5a058fb263bc2449dbc9d7529841

Patches applied

…on-view Utf8s, and test does pass (due to coercion?)
@wiedld
Copy link
Collaborator Author

wiedld commented Dec 12, 2024

@alamb -- I had to add a single additional patch commit, due to an upstream commit from oct-8.

The test case correctly shows that bit_length(utf8view) statement should fail.
However, I'm not really sure how this statement was properly failing before (on datafusion main)?

Steps I took to debug:

  1. without changes:
    • we get a utf8 view type for the column, post coercion
    • the statement bit_length(utf8view) was not failing on our DF upgrade branch.
  2. I changed:
    • made the signature of bit_length only accept utf8 and large utf (not the views), since I think that oct-8 upstream commit made a wrong change. Note that the evoke does not handle view types.
  3. but the tests still passed.
    • it looks like type coercion changed the utf8 view to large utf8.

So this branch now passes all DF tests, but I'm still unsure how DF main is ok with this test. (This was also called out in the PR itself.) 🤔

@alamb
Copy link
Collaborator

alamb commented Dec 12, 2024

Patches applied

Thank you for this list

However, I'm not really sure how this statement was properly failing before (on datafusion main)?

I believe this was fixed in a newer version of arrow-rs that was released after that DataFusion commit

Since IOx is now running a newer arrow in my mind that explains the test difference

@alamb
Copy link
Collaborator

alamb commented Dec 12, 2024

We are still manually patching upstream commits, for these fixes:

BTW I have added a discussion item for our next meeting about these patches (aka how should we be thinking about them upstream)

@wiedld
Copy link
Collaborator Author

wiedld commented Dec 19, 2024

We are now on Oct-30th. Closing this PR.

@wiedld wiedld closed this Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants