Respect two inference-affecting settings in the schema inference cache key - #113533
Respect two inference-affecting settings in the schema inference cache key#113533groeneai wants to merge 9 commits into
Conversation
…e key The schema inference cache key's additional_format_info component is an explicitly maintained allow-list of settings, appended to by hand whenever a setting is found to change an inferred type. Two such settings were never added, so for a source whose schema is cached the setting value of the first query decides the type every later query sees. input_format_try_infer_exponent_floats selects the float parser in tryReadFloat (SchemaInferenceUtils.cpp:1013-1018): with it off on a non-JSON escaping rule, tryReadFloatTextExtNoExponent stops at the exponent, has_fractional stays false, and the field completes to String instead of Float64. max_parser_depth is read by the generic tryInferDataTypeForSingleFieldImpl (:1425), so over the limit inference throws TOO_DEEP_RECURSION. The consequence is user-visible in both channels: a TSV file holding 1e5 is inferred as Float64 or String depending only on which setting value ran first, and the poisoned query returns 100000 where a cold query returns 1e5. For max_parser_depth only the permissive direction is reachable, because a throwing inference caches nothing: a low limit that should throw silently succeeds once a high-limit query has warmed the cache. Placement follows where each setting is verdict-changing rather than symmetry. max_parser_depth goes in getAdditionalFormatInfoForAllRowBasedFormats because the check sits in the generic single-field entry point, and because BSONEachRow and MsgPack call only that function while enforcing the setting themselves. The exponent setting goes in the Escaped/Raw and CSV arms of getAdditionalFormatInfoByEscapingRule plus a new Quoted arm reached by Values and MySQLDump. It is deliberately absent from the JSON arm: tryReadFloat short-circuits it there via is_json, so it cannot change a JSON verdict, and the new test asserts JSON keeps a single cache entry for both values so a later change cannot add it for symmetry. Three formats cannot inherit from those two functions and are fixed at their own getter. Form registered no cache getter at all, so its additional_format_info was the empty string and every keyed setting collided; it now registers one for the Raw rule its schema reader actually infers with. Parquet's getter omitted max_parser_depth, which Parquet/SchemaConverter.cpp reads and throws on; ORC already keys on that setting, and is the precedent. Template's getter looped over its row format's escaping rules but passed settings.regexp.escaping_rule, a Regexp setting, so it keyed on the wrong arm. precise_float_parsing and allow_number_leading_zeros, also named in the original report, are deliberately not added. The former was measured to change neither the inferred type nor the read value, and inference isolates itself from it. The latter is not a declared setting: its only writer hardcodes it on a hive-local FormatSettings whose consumers never reach a schema cache function, so no value of it can appear in a key. Parquet's getter still omits four other settings its inference path reads; those were not measured for order-dependence and are left alone, so this change does not make the cache key complete. Growing the key invalidates cached entries once per source after upgrade. The cache is in-memory and process-local, so this is a warm-up re-inference rather than a compatibility break, matching commit 8a88ad5, which changed the same string with no migration. The Template fix additionally corrects keys for users whose format_regexp_escaping_rule differs from their row format's rules. No setting default changes, so no SettingsChangesHistory entry is due. The new test probes both orders per carrier because an order-insensitive test cannot detect this class. Its fixtures are aged with touch: SchemaCache discards an entry when the source's mtime is at least its registration time and both are whole seconds, so a fixture written in the same second as the first query is re-inferred and nothing is cached.
Internal second-model reviewTwo independent reviews of this change: my own cold read of the resulting code (done before My own pass found no blocker and no major in the code. Three items, with verdicts:
💡 Five formats' cache keys still omit 💡 The unordered reads of Independently re-derived while reviewing, each reproducing exactly: the exponent setting is |
Validation gate (a–i)
Mutation matrix. Eight mutants, each built with the Build ID asserted to move and the server
M1 and M7 are independently pinned, as required: neither reddens the other's arm. M2 cross-trips Runs. 50 randomized runs of the new test and of each of the four touched tests: Deliberately out of scope. |
|
cc @vitlibar @scanhex12, could you review this? The schema inference cache key is a hand-maintained allow-list of settings, and |
alexey-milovidov
added
the
can be tested
|
Workflow [PR], commit [fe3502f] Summary: ✅
AI ReviewSummaryThis PR fixes schema-inference cache key collisions by hashing the settings that actually change inferred types for row-based text formats, Missing context / blind spots
Final Verdict✅ No new blockers or majors found in the current PR head. The previously reported cache-key omissions are fixed in the current code. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 79/81 (97.53%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…t_parser Both settings change the type schema inference produces for Parquet, so a schema cached under one value was reused for the other and the first query's value decided the type every later query saw. input_format_parquet_local_time_as_utc selects the timezone of the inferred DateTime64 for a timestamp column that is not adjusted to UTC (Parquet/SchemaConverter.cpp:1140). With the cache warm this also changes the value read back, not only the type name: on the not_utc.parquet fixture a DESC at =1 followed by a read at =0 returns 2023-01-01 12:00:00 where reading with the cache disabled returns 13:00:00. input_format_parquet_allow_geoparquet_parser decides whether a GeoParquet geometry column is inferred as a geo type or as its raw String representation (SchemaConverter.cpp:47), so on 03445_geoparquet_null_linestring.parquet a poisoned entry reports LineString where Nullable(String) is correct, or the reverse. The sibling native ORC reader already keys skip_columns_with_unsupported_types for the same reason (NativeORCBlockInputFormat.cpp:3001, added in e88e3c6 with regression test 04514). Reported by the AI reviewer on ClickHouse#113533. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
input_format_parquet_skip_columns_with_unsupported_types_in_schema_inference decides whether a column whose Parquet type is not implemented is dropped from the inferred schema or the file is rejected, but it was absent from the Parquet schema-inference cache key. A source whose schema was cached by a permissive query was therefore reused by a later strict query, which silently got the reduced schema instead of the exception it should have raised. Measured on the fixture added here, a Parquet file with a VARIANT-typed column, which is a valid logical type the reader does not implement. With the cache disabled the verdict flips: at 1 the column is dropped and DESC reports only `id`, at 0 inference throws INCORRECT_DATA. With the cache enabled and the permissive query first, the following strict query returned the cached schema in 5 of 5 runs, and the key was byte-identical at both setting values. Only this direction is a carrier, because the strict query throws and a throwing inference caches nothing. The native ORC reader already keys its equivalent setting and 04514 is its regression test, so this brings Parquet in line with it. schema_inference_make_json_columns_nullable also reaches an inferred type from this getter but is deliberately left alone: PR 102499 adds that same field to this same format-info string. Test arms cover the poisoning order and a control asserting that the strict query throws on its own, plus the cache-key string itself. Dropping the new field again reddens those two arms and no other.
The order arm greps for the exception the strict query must raise, which it also raises when nothing was cached at all: measured, the arm still reports 1 with schema_inference_use_cache_for_file = 0. Reading the entry back shows the permissive query left one, keyed on its own setting value, so the strict query bypassed a cache entry rather than finding none. Dropping the field from the getter now reddens this arm too, and still no arm of another setting.
…llable schema_inference_make_json_columns_nullable decides whether a required JSON column is inferred as JSON or Nullable(JSON), but it was absent from the Parquet schema-inference cache key, so a cached source returned whichever the first query had inferred. Measured on the fixture added here, a Parquet file with a required JSON column. With the cache disabled the type flips between JSON and Nullable(JSON). With the cache enabled the second query returned the first query's type in 3 of 3 runs in both orders. The column must be required: an optional one is nullable at either value, so it cannot show the difference. PR 102499 adds this same field to this same format-info string. Waiting for it was the earlier plan here, but it has been open since April across 176 files and already conflicts with this branch in this file, so this key stays incomplete for as long as that PR takes. The conflict is a one-line resolution either way and is not avoided by leaving the setting out. Dropping either of the last two fields from the getter reddens only that setting's own arms, so both are pinned independently.
An entry being present does not prove a later query read it: with schema_inference_use_cache_for_file off, every query re-infers the correct type and rewrites the same entry, so the whole test still passes. Measured, SchemaInferenceCacheSchemaHits is 1 with cache reads live and 0 with them bypassed, so repeating one query at unchanged settings and requiring a hit closes that gap.
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units10 translation units recompiled, 26 s compile time in total, 9 of them have a recent master baseline. |
00900_long_parquet_load_2 sweeps tests/queries/0_stateless/data_parquet/ with find and loads every file it sees, so the two fixtures this PR added there were picked up by it. parquet_variant_logical_type.parquet carries a VARIANT logical type on a leaf column, which the reader rejects with INCORRECT_DATA by design; that is what the 04757 skip_columns_with_unsupported_types arm needs. Loading it in 00900 prints that exception into stdout, which clickhouse-test reports as "having exception in stdout" before it ever reaches the reference comparison. That masked a second failure: both new fixtures are absent from the reference, so the file also failed "result differs with reference". Excluding both matches how every other fixture that exists for another test is handled here, including the four 04065 wrapper fixtures and the 04099 dictionary memory fixture, neither of which throws. The alternative, regenerating the reference, cannot work for the VARIANT file: the exception check runs first, and no currently swept file emits an exception. Verified with a driver that replays 00900's loop and applies both clickhouse-test predicates. Before: FAIL on the exception check at the VARIANT file, plus +9 reference lines. After: both checks pass, output byte-identical to the reference. Excluding only one file leaves the other failing on its own reason, so both entries are load-bearing. 00900 is tagged no-debug and cannot run through the runner in a debug build, which is why CI only reports it on arm_binary.
…ferent_types The JSON escaping-rule branch of getAdditionalFormatInfoByEscapingRule hashes nine settings.json fields but not infer_array_of_dynamic_from_array_of_different_values, which transformInferredJSONTypesIfNeeded and transformFinalInferredJSONTypeIfNeededImpl consult when choosing between Array(Dynamic) and an unnamed Tuple for a heterogeneous JSON array. Both values therefore shared one cache entry, so the first query for a source decided the array type every later query saw. Measured on a mixed-type array, cache off: the setting flips the type between Array(Dynamic) and Tuple(Nullable(Int64), Nullable(String), Array(Nullable(Int64))). With the cache on it was order-dependent in both directions, 5 of 5 runs per order, with the recorded key byte-identical either way. After the change each query gets the type it asked for and the source keys two entries instead of one. This closes the shared getter: across all five escaping-rule branches, every settings.json, settings.csv and settings.tsv field the inference implementation reads is now hashed, so the read-but-unkeyed set is empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…amic arms The two order arms re-infer at each setting value either way, so they hold even if cached schemas were never read back and the key row only proves two entries exist. Repeating one query at unchanged settings pins the read: SchemaInferenceCacheSchemaHits is 1 with the cache live and the event is never incremented when reads are bypassed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI finish ledger — fe3502fEvery failure below has an owner: a fixing PR (mine or external), or a full-effort fix task 175 check-runs, all completed: 158 success / 17 skipped / 0 failure.
Session id: cron:our-pr-ci-monitor:20260806-133000 |
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed the inferred column type depending on the order of previous queries for a source whose schema is cached. Settings that change an inferred type were missing from the schema inference cache key, so the first query's value decided the type every later query saw:
input_format_try_infer_exponent_floats,max_parser_depth,input_format_json_infer_array_of_dynamic_from_array_of_different_types, and forParquetalsoinput_format_parquet_local_time_as_utc,input_format_parquet_allow_geoparquet_parser,input_format_parquet_skip_columns_with_unsupported_types_in_schema_inferenceandschema_inference_make_json_columns_nullable. Also registers the missing cache key getter forFormand makes theTemplategetter use its own row format's escaping rule.Description
The key's
additional_format_infocomponent is a maintained allow-list, and settings that change aninferred type were never added, so for a cached source the first query's value decides the type every
later query sees.
With
1e5in a TSV file,input_format_try_infer_exponent_floats = 1infersFloat64and= 0infers
String, and in either order both queries return the first query's type.max_parser_depthis affected in the permissive direction only: a low limit that should throw
TOO_DEEP_RECURSIONsucceeds once a high-limit query warmed it.
Three formats cannot inherit the fix from those two functions and are fixed at their own getter.
Formregistered no getter, so itsadditional_format_infowas empty and every keyed settingcollided.
Parquet's getter omittedmax_parser_depth, which its schema converter throws on, pluslocal_time_as_utc,allow_geoparquet_parser,skip_columns_with_unsupported_types_in_schema_inferenceandschema_inference_make_json_columns_nullable;Template's getter passedformat_regexp_escaping_ruleinsteadof its row format's own rule, keying the wrong arm.
Placement follows where each setting is verdict-changing:
max_parser_depthin the all-formatsstring, the exponent setting in the
Escaped/Raw,CSVand a newQuotedarm but not in JSON, andinfer_array_of_dynamic_from_array_of_different_typesin the JSON arm, where it selectsArray(Dynamic)or an unnamedTuplefor a heterogeneous array.Cached schemas are re-inferred once per source after upgrade; the cache is in-memory and
process-local, so this is a warm-up cost, as in
8a88ad5dc4d24d7. PRs #104816, #110766 and #102499 conflict textually only.