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

CAY-2847 Improve duplicate select column detection when using order by #610

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jugen
Copy link
Contributor

@Jugen Jugen commented Feb 29, 2024

The current implementation has three problems:

  1. It fails when a column has an alias
  2. It builds the whole SQL column string before comparing
  3. It doesn't seem very robust in preventing false results

This PR replaces the current implementation with a visitor pattern that fails fast.
The visitor extracts each node's parts into a List which is progressively compared to the parts extracted from the order node.
Each item must match in position and content in both lists for them to match and will abort comparing as soon as possible.

Copy link
Member

@stariy95 stariy95 left a comment

Choose a reason for hiding this comment

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

This seems a better solution than what we already have, so I'll merge it as soon I'll be back at Cayenne. There's a minor wish, but that's optional.

Looking at the whole feature, I dream that we'll have a proper comparison API for the SQL nodes one day, and that would make our life so much easier 😄

@Jugen
Copy link
Contributor Author

Jugen commented Mar 19, 2024

I think that there are two tests needed but I don't know how to add them to the current test setup in OrderingStageTest

  1. Test that no duplicate column is added for an aliased select column
  2. Test that no duplicate column is added for a complex operator column

These are not critical tests, just sanity checking that the code does what is expected.

@Jugen Jugen force-pushed the improve-duplicate-column-detection-for-order-by branch from 3d4425b to 29dadda Compare May 17, 2024 06:50
@Jugen
Copy link
Contributor Author

Jugen commented May 17, 2024

I've rebased & squashed commits for this PR, and updated the release-notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants