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

cql3/prepare_expr: fix partially preparing function arguments #14786

Closed
wants to merge 1 commit into from

Conversation

cvybhu
Copy link
Contributor

@cvybhu cvybhu commented Jul 21, 2023

Before choosing a function, we prepare the arguments that can be prepared without a receiver. Preparing an argument makes its type known, which allows to choose the best overload among many possible functions.

The function that prepared the argument passes the unprepared argument by mistake. Let's fix it so that it actually uses the prepared argument.

Before choosing a function, we prepare the arguments that can be
prepared without a receiver. Preparing an argument makes
its type known, which allows to choose the best overload
among many possible functions.

The function that prepared the argument passes the unprepared
argument by mistake. Let's fix it so that it actually uses
the prepared argument.

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
@cvybhu cvybhu requested a review from avikivity July 21, 2023 13:53
@cvybhu
Copy link
Contributor Author

cvybhu commented Jul 21, 2023

I didn't add any unit tests, because I couldn't find any cases that would be affected by this bug.

Why aren't there any?
I originally added the partial preparing because test_assignment didn't work well on unprepared arguments. In particular I remember that trying to run test_assignment on an unprepared unresolved_identifier caused an error, so all columns had to be prepared before choosing a function.

Now I see that test_assignment prepares unprepared expressions, so this issue doesn't appear.
I'm not sure how valid this is. prepare_expression is supposed to throw an error when the receiver type doesn't match, but test_assignment shouldn't throw any exceptions on type mismatch, it should return a value.

We can consider some changes in this area, but for now let's fix the buggy code, so that it does what it was supposed to.

@scylladb-promoter
Copy link
Contributor

Copy link
Contributor

@nyh nyh left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have to admit I didn't understand why it's impossible to write a unit test that would have caught this bug, but I agree it looks like a straighforward typo that should be fixed so I don't need a test to be convinced.
IIUC, this is a change to very new code (8d3d8ee) so it won't need a backport so I'll avoid my usual request to open issues when fixing bugs, but in general - we should.

patjed41 pushed a commit to patjed41/scylladb that referenced this pull request Jul 27, 2023
Before choosing a function, we prepare the arguments that can be
prepared without a receiver. Preparing an argument makes
its type known, which allows to choose the best overload
among many possible functions.

The function that prepared the argument passes the unprepared
argument by mistake. Let's fix it so that it actually uses
the prepared argument.

Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>

Closes scylladb#14786
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants