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

memo.Literal has different type than lookup #1851

Merged
merged 5 commits into from
Jun 26, 2023
Merged

memo.Literal has different type than lookup #1851

merged 5 commits into from
Jun 26, 2023

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Jun 26, 2023

This panics on dolt:

CREATE TABLE tab2(pk INTEGER PRIMARY KEY, col0 INTEGER, col1 FLOAT, col2 TEXT, col3 INTEGER, col4 FLOAT, col5 TEXT);
CREATE UNIQUE INDEX idx_tab2_0 ON tab2 (col1 DESC,col4 DESC);
CREATE INDEX idx_tab2_1 ON tab2 (col1,col0);
CREATE INDEX idx_tab2_2 ON tab2 (col4,col0);
CREATE INDEX idx_tab2_3 ON tab2 (col3 DESC);

INSERT INTO tab2 VALUES(0,344,171.98,'nwowg',833,149.54,'wjiif');
INSERT INTO tab2 VALUES(1,353,589.18,'femmh',44,621.85,'qedct');

SELECT pk FROM tab2 WHERE ((((((col0 IN (SELECT col3 FROM tab2 WHERE ((col1 = 672.71)) AND col4 IN (SELECT col1 FROM tab2 WHERE ((col4 > 169.88 OR col0 > 939 AND ((col3 > 578))))) AND col0 >= 377) AND col4 >= 817.87 AND (col4 > 597.59)) OR col4 >= 434.59 AND ((col4 < 158.43)))))) AND col0 < 303) OR ((col0 > 549)) AND (col4 BETWEEN 816.92 AND 983.96) OR (col3 BETWEEN 421 AND 96);

The PutField function expects the value to match the tuple descriptor exactly, and will panic if it does not.
The section of code in memo that creates a new range uses the type from the expression, but in other places it uses the index column expression types.

An alternative solution would be to have some logic in dolt to convert to the corresponding sql.Type based off the val.Enc

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.

i think this is the right point fix, key for index has to be the index type. We messaged about testing, maybe get the memory implementation to also error if there's a lookup type incompatibility.

@jycor jycor merged commit 2fc0613 into main Jun 26, 2023
@jycor jycor deleted the james/memo branch July 21, 2023 17:27
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.

2 participants