Fix #380, bug in code that reads Enums #454

fotonick · 2025-03-04T09:22:21Z

In turning a ValueRef::Enum to a Value, the code was inappropriately indexing with the row index directly into the dictionary's string array, so iterating any query's results was effectively just listing the Enum variants in order until row index > number of variants and it panicked. Fix by getting the row/col value (the dictionary key) and using that to index into the dictionary's string array.

You can see it in all three branches of the match statement at value_ref.rs#L245. These call DictionaryArray.values(), whose docstring says clearly that it returns the dictionary values.

Note that this code was covered by a test, but the expected value was hardcoded incorrectly to make the previous implementation pass. Proof that the old expected value was incorrect and the new one is correct:

D SELECT medium_enum FROM test_all_types();
┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│                                                                 medium_enum                                                                  │
│ enum('enum_0', 'enum_1', 'enum_2', 'enum_3', 'enum_4', 'enum_5', 'enum_6', 'enum_7', 'enum_8', 'enum_9', 'enum_10', 'enum_11', 'enum_12', …  │
├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ enum_0                                                                                                                                       │
│ enum_299                                                                                                                                     │
│ NULL                                                                                                                                         │
└──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

In turning a ValueRef to a Value, the code was indexing with the row index directly into the dictionary's string array, so iterating any query's results was effectively just listing the Enum variants in order until row index > number of variants and it panicked. Fix by getting the row value (key) and using that to index into the dictionary's string array.

The test tests for correct value extraction, based on built-in examples in DuckDB. The old expected value was flatly incorrect, as can be seen in duckdb directly with `SELECT medium_enum FROM test_all_types();`.

fotonick · 2025-03-04T09:27:16Z

I should mention that cargo test --features bundled passes, as does ASAN_OPTIONS=detect_leaks=1 ASAN_SYMBOLIZER_PATH=/opt/homebrew/Cellar/llvm/19.1.7/bin/llvm-symbolizer cargo test --features bundled -- --nocapture.

Maxxen · 2025-03-12T09:46:48Z

Hello! Thanks a lot for the PR! I've included these changes as part of #462, so I'm going to go ahead and close this.

fotonick added 2 commits March 4, 2025 00:51

Fix incorrect Enum test

9f4dbec

The test tests for correct value extraction, based on built-in examples in DuckDB. The old expected value was flatly incorrect, as can be seen in duckdb directly with `SELECT medium_enum FROM test_all_types();`.

shritesh mentioned this pull request Mar 7, 2025

Do not pass both stdlib flags. #455

Closed

fotonick force-pushed the main branch from 159a7d1 to 9f4dbec Compare March 10, 2025 20:38

Maxxen closed this Mar 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #380, bug in code that reads Enums #454

Fix #380, bug in code that reads Enums #454

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix #380, bug in code that reads Enums #454

Fix #380, bug in code that reads Enums #454

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!