Repository navigation
Match external write option names case-insensitively - #869
Conversation
DuckDB accepts COPY options in any case, but the external materialization only recognized lower-case names. An options key such as PARTITION_BY was passed through unchanged, so a multi-column value was not wrapped in parentheses, the read-back view pointed at the partition directory instead of the files below it, and the default location got a file extension. An upper-case FORMAT was followed by a second, inferred format option and DuckDB rejected the duplicate. Lower-case the option names before the adapter inspects them, and look up partition_by case-insensitively when picking the default location.
|
thanks, this looks great! one thing: when two keys differ only by case they now get silently merged. something like how do you think we should handle that? my lean is to raise a compiler error when two keys normalize to the same name (plus a unit test for it), but I'm open to a defined precedence if you think that's better. |
|
Agreed, an error is better than a precedence rule. A config that sets the same option twice is almost always a mistake, and erroring keeps the old failure instead of silently picking one value. Done in d90e289: With your example ( Unit tests (196) and |
jwills
left a comment
jwills
left a comment
There was a problem hiding this comment.
LGTM, thanks for the quick turnaround on this!
Fixes #232
DuckDB accepts
COPY ... TOoptions in any case, but theexternalmaterialization only recognized lower-case option names. In #232 a model withoptions={"PARTITION_BY": "date"}wrote the partitioned files and then failed creating the__dbt_intview, becauseexternal_read_locationdid not see the partition option and read the directory instead of the files below it. The workaround in that thread was to switch to lower-casepartition_by.On current
masterthe same problem shows up in a few places:options{"PARTITION_BY": "id"}<location>instead of<location>/*/*.parquet{"PARTITION_BY": "id, name"}Unrecognized option "name" for parquetpartition_by (id, name){"FORMAT": "csv"}format parquetis appended:Unexpected duplicate option "format"format csv{"PARTITION_BY": ...}with nolocation.parquetsuffixpartition_byChanges:
external_write_optionslower-cases the option names in place before inspecting them, so the materialization's later reads ofrendered_options['format']andrendered_options['compression']see them too. The quoting check already comparedk.lower(); it now compares the normalized key.external_read_locationlooks the options up case-insensitively (without mutating the caller's dict). This also coversregister_upstream_external_models.external_locationfindspartition_byin any case when choosing the default location.Tests:
tests/unit/test_external_utils.py: upper/mixed-case cases forexternal_write_optionsandexternal_read_location, plus a check that the dict is normalized in place.tests/functional/adapter/test_write_options.py: two models usingPARTITION_BY/CODECandFORMAT/DELIMITER, checked against the seed withcheck_relations_equal, and a check that partitioned outputs default to a directory without an extension. Both fail onmasterand pass with this change.