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

Avoid cloning in TreeNode.children_nodes() implementations where possible using Cow #132

Merged
merged 1 commit into from
Dec 31, 2023

Conversation

peter-toth
Copy link

Avoid cloning in TreeNode.children_nodes() implementations where possible using Cow

@viirya
Copy link
Owner

viirya commented Dec 30, 2023

Although there is test failure:

SELECT TIMESTAMPTZ '2022-01-01 01:10:00 America/Los_Angeles' as ts_geo
[Diff] (-expected|+actual)
    2021-12-31T14:10:00Z
-   2021-12-31T19:10:00Z
+   2021-12-31T18:10:00Z
    2021-12-31T23:10:00Z
    2022-01-01T09:10:00Z
at test_files/timestamps.slt:1730

But I cannot reproduce it locally. Maybe it is flaky or due to forked repo setting? 🤔 Seems an unrelated failure.

@peter-toth
Copy link
Author

Although there is test failure:

SELECT TIMESTAMPTZ '2022-01-01 01:10:00 America/Los_Angeles' as ts_geo
[Diff] (-expected|+actual)
    2021-12-31T14:10:00Z
-   2021-12-31T19:10:00Z
+   2021-12-31T18:10:00Z
    2021-12-31T23:10:00Z
    2022-01-01T09:10:00Z
at test_files/timestamps.slt:1730

But I cannot reproduce it locally. Maybe it is flaky or due to forked repo setting? 🤔 Seems an unrelated failure.

Oh, I didn't notice the failures.
But, that's weird. I don't see them locally either.
And there is this clippy error too:

error: unused import: `exec_err as _exec_err`
   --> datafusion/common/src/error.rs:561:9

but I didn't touch that file.

@viirya
Copy link
Owner

viirya commented Dec 30, 2023

Let me merge this first and see how CI works in DataFusion repo.

@viirya viirya merged commit 3b8a7ad into viirya:refactor_treenode2 Dec 31, 2023
16 of 22 checks passed
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.

2 participants