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

Fix issue where case expression is evaluated too early #1420

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

PavelSafronov
Copy link
Contributor

Fix issue where case expression is evaluated and the result is being treated as a literal.

Fixes dolthub/dolt#4716

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

LGTM, it is usually simpler to avoid new test setup harnesses when we can avoid it, but we don't appear to have any enginetests that touch enums? Which seems kind of concerning.

Copy link
Contributor

@fulghum fulghum 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! Nice work with this fix. ⚡

Max had a good comment about spinning up a new enginetest Test method... okay with me, but you should also add that to dolt, so that dolt_engine_test runs that new Test method. Alternatively... it might be a little simpler to just put this test in the (unorganized grab bag) queries.ScriptTests. Either way works for me.

enginetest/queries/queries.go Outdated Show resolved Hide resolved
@PavelSafronov
Copy link
Contributor Author

Thanks for the review, @max-hoffman and @fulghum!

we don't appear to have any enginetests that touch enums? Which seems kind of concerning.

Looks like we have enum tests in sql/enumtype_test.go, but yeah, there's a lack of table enum tests.

Alternatively... it might be a little simpler to just put this test in the (unorganized grab bag) queries.ScriptTests.

That seems like a good place, I'll move the new tests to that location.

I found the setup_data.sg.go changes to be strange and honestly, I don't get them. What is the benefit of translating plain files of SQL (no intellisense, no type checking, etc.) into go files?

@max-hoffman
Copy link
Contributor

I found the setup_data.sg.go changes to be strange and honestly, I don't get them. What is the benefit of translating plain files of SQL (no intellisense, no type checking, etc.) into go files?

There might not be a good one! The current structure is the result of rewriting the previous format, which used the database, table, and schema APIs in Go to setup tests. The current format uses SQL queries to do setup, which I think is a comparative improvement. The duplication provides an IR that we can easily inline in code; passing around batched blocks SQL strings for custom bootstrapping was seen as undesirable at the time. Alternative IRs with less duplication might look like at Aaron's server-driver yaml, the correctness test format, and cockroach's test format. Note that all of these require investments in testing infrastructure.

@PavelSafronov PavelSafronov merged commit f1a8a25 into main Nov 22, 2022
@Hydrocharged Hydrocharged deleted the pavel/dolt-4716 branch March 29, 2023 01:09
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.

Simple case statement doesn't work with enums
3 participants