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

Feat/make dfschema wrap schemaref #8905

Conversation

matthewmturner
Copy link
Contributor

Which issue does this PR close?

Closes #4680

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@matthewmturner
Copy link
Contributor Author

@alamb FYI adding for visibility. Still plenty of work to do but publishing in case theres any feedback along the way.

@matthewmturner
Copy link
Contributor Author

I have come across several methods which have been deprecated for a while now (i.e. Column.normalize_with_schema (since v20), DFSchema::new (since v7), and DFSchema.index_of (v8)). Is it safe to remove these now?

@matthewmturner
Copy link
Contributor Author

As a status update this week has been slow as I've been on site for company hackathon. I expect to pick up again this weekend or early next week.

@matthewmturner
Copy link
Contributor Author

@alamb would you be able to give this a quick overview to make sure its going in the right direction?

@alamb
Copy link
Contributor

alamb commented Jan 29, 2024

@alamb would you be able to give this a quick overview to make sure its going in the right direction?

Yes I will try to do so today or tomorrow

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alamb would you be able to give this a quick overview to make sure its going in the right direction?

I think it is is @matthewmturner -- thank you 🙏

Is there anything thing I can do to help (e.g. help port an API or debug tests?)

/// Inner Arrow schema reference.
inner: SchemaRef,
/// Optional qualifiers for each column in this schema. In the same order as
/// the `self.inner.fields()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@matthewmturner
Copy link
Contributor Author

@alamb great, thanks for checking it out. all good for now. i just got datafusion common to build after all these changes and now cleaning up a few failing tests.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Feb 4, 2024
@matthewmturner
Copy link
Contributor Author

@alamb sorry this is taking a while, i havent had a significant amount of time to crank this out so im basically working on it bit by bit every morning i get the chance. hopefully this isnt slowing anything down on your side.

@alamb
Copy link
Contributor

alamb commented Feb 9, 2024

@alamb sorry this is taking a while, i havent had a significant amount of time to crank this out so im basically working on it bit by bit every morning i get the chance. hopefully this isnt slowing anything down on your side.

Thank you for keep plugging away. This is not blocking anything on my end (cc @comphead who maybe is more actively working on the planning speed)

I think I would classify this PR as important but not urgent (much like splitting up functions into crates -- e.g. #9113).

Thank you for continuing to push

@haohuaijin
Copy link
Contributor

Hi @matthewmturner I've noticed that this PR has been open for quite a while and it seems like you've already done a lot of work on it. I'm really interested in this improve and would like to contribute if possible. If you're busy with other commitments or if you could use some help to finish this up, I'd be more than happy to assist. Of course, I want to respect your work, so please let me know how you would feel about this. Thanks for all the effort you've already put into this PR!

@matthewmturner
Copy link
Contributor Author

@haohuaijin its very timely that you reached out on this I was actually about to post an update here that while I had planned to continue working on this when I had time I was going to be prioritizing some other work before this so I was open to others helping to push this forward during that time. So I would be more than happy to have you assist getting this over the finish line :)

@haohuaijin
Copy link
Contributor

@matthewmturner, thank you very much for your reply and openness to my participation in this PR. I'm excited for the opportunity to help move this project forward.
In the next work, I have two possible ways to proceed. One is that I push my updates to your repository, and then you can merge my PR into the current PR. The other is that I create a new PR based on your PR. I was wondering if you have any preference between these two methods? Or do you have any other suggestions?

@matthewmturner
Copy link
Contributor Author

I dont want to hold you up if I am unable to merge your PR into mine for whatever reason so I think creating a new PR is probably the better approach here. I look forward to seeing your work on this :)

@haohuaijin
Copy link
Contributor

@matthewmturner, thank you very much for your advice and support. I will follow your suggestion and create a new PR based on your PR. I'll start working on it as soon as possible and let you know when it's complete. Thanks again for your help and support.

@haohuaijin
Copy link
Contributor

@matthewmturner and @alamb, I've created a new PR (#9595) based on the original one. I welcome your review and feedback at any time. I will continue to work on #9595. If you have any questions or suggestions, please feel free to share them. I appreciate your guidance and support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make DfSchema wrap SchemaRef
3 participants