-
Notifications
You must be signed in to change notification settings - Fork 78
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
Fix bugs in moveColumns and renameColumns #5193
Conversation
859086d
to
3b5f066
Compare
3b5f066
to
c0ba6a1
Compare
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.
The Python changes LGTM
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.
Python LGTM
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.
I really like the new Javadoc and the new tests, helped me understand the code much better. I wonder if we can add similar move+rename examples in the official docs as well since I couldn't find such an example there.
Suggested a small correction in javadoc at one place. Would defer the discussion about semantics to the experts.
Co-authored-by: Shivam Malhotra <malhotraashivam@gmail.com>
d8e01d3
Co-authored-by: Chip Kent <5250374+chipkent@users.noreply.github.com>
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.
.
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
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.
Don't hate me
engine/table/src/main/java/io/deephaven/engine/table/impl/BaseTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
Outdated
Show resolved
Hide resolved
b192365
to
f57851b
Compare
Labels indicate documentation is required. Issues for documentation have been opened: Community: https://github.com/deephaven/deephaven.io/issues/3750 |
Fixes #4824.
There were a few odd behaviors of the existing replace columns implementation. This hopefully makes behavior well defined and consistent. I've also added tests; which apparently were quite useful in extracting bugs.
I've explicitly disallowed having multiple entries for the same source or the same destination. You can lose columns by renaming some column to an existing column name; it was easy to support and there may be legitimate uses.
Nightlies: https://github.com/nbauernfeind/deephaven-core/actions/runs/8085227568