Consolidate catalog types and split feature oracle - #1781
Merged
Conversation
Contributor
Version-Control Performance CeilingsRuns: median of 3 executions per benchmark, excluding fixture setup. The
|
Contributor
Sysbench-Style Benchmark (composite PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
Contributor
Sysbench-Style Benchmark (TEXT PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
Contributor
Sysbench-Style Benchmark: Doltlite vs SQLiteIn-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
Contributor
Sysbench-Style Benchmark (BLOB PK): Doltlite vs SQLiteCompanion to the classic Sysbench-Style Benchmark. Every workload here In-MemoryReads
Writes
File-BackedReads
Writes
File-Backed (autocommit)Each statement runs as its own transaction — exposes per-commit ReadsReads have no commit cost; these are the same SQL files as the
Writes
100000 rows, median of 5 invocations per test; autocommit writes use 9, workload-only timing via host monotonic clock when available. Performance Ceiling Check (2.5x individual, 2x average; autocommit writes: 10x / 5x)All tests within ceilings. |
core-sql failed on CI with "bigsort: did not produce summary line" after ~452s — the 450s per-file timeout killing the heavy CREATE INDEX over 300k blobs. That is a load flake, not a catalog-types regression. Bump per-file timeout 450→600 and the regression job ceiling 20→25 min.
Collaborator
Author
CI failure analysis (
|
Per-file testfixture timeout stays at 600s for bigsort under PROLLY_CHECK; leave the overall job ceiling at 20m and split the bucket later if wall time consistently exceeds it.
|
SummaryCoverage spans build configurations, catalog persistence and reload behavior, branching and merging, broad feature comparisons, and adversarial handling of mismatches, partial results, invalid inputs, and regression failures. Core product workflows appear broadly healthy, but the validation layer does not reliably detect when its sourced scenarios are missing. Merge with caution — the PR introduces a medium-severity coverage-integrity defect that can report success without running the intended scenarios, weakening confidence in future regression detection. Separate medium-severity catalog and regression failures are unrelated to this PR and remain flag-for-later observations. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Schema diff rejects WORKING reference
Evidence Package🟡 Regression bucket crashes before summary
Evidence Package🟡 Cumulative regression run crashes unexpectedly
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reject a silently empty sourced suite
What failed: The empty-suite guard test expected a sourced suite with no executable oracle cases to fail, but the copied runner printed 0 passed, 0 failed and returned exit 0.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: An empty or missing sourced suite can report success without executing any cases, allowing regressions in the intended feature coverage to reach users unnoticed.
- Steps to Reproduce:
- Temporarily replace the five sourced feature-family files with empty files while leaving the runner unchanged.
- Run test/vc_oracle_feature_interaction_test.sh with the normal DoltLite and Dolt executable arguments.
- Observe the 0 passed, 0 failed summary and exit code 0, then restore the family files and confirm the normal run executes 752 cases.
- Stub / mock content: The test intentionally substituted temporary empty family sources to model missing coverage; no product mocks, route interceptions, or production-data bypasses were used.
- Code Analysis: At line 11, pass and fail both start at zero. Lines 55-57 source the five family files, but sourcing an empty or incomplete fragment invokes no oracle and therefore never reaches vc_oracle_assert_match. Lines 60-64 print the counters and exit nonzero only when fail is greater than zero, so zero executed cases are treated as success. The helper's both-empty output check at test/lib/vc_oracle_common.sh:56-63 does not protect this path because it only runs when an oracle call exists. The smallest fix is to add a final pass + fail -gt 0 check before the existing fail check and return a nonzero status when no cases ran.
- Why this is likely a bug: The reproduced result is independent of the browser or external services and follows directly from the runner's control flow: removing all sourced assertions leaves both counters at zero, yet the success condition checks only fail. Because this runner is intended to prove that all five feature families remain covered after the PR's split, silently accepting zero cases defeats that safety check; adding an explicit nonzero-case guard is a targeted fix.
Relevant code
test/vc_oracle_feature_interaction_test.sh:11
pass=0; fail=0test/vc_oracle_feature_interaction_test.sh:55-64
for family in merge history schema query stress; do
source "$SCRIPT_DIR/lib/vc_oracle_feature_interactions/$family.sh"
done
echo ""
echo "=== Results: $pass passed, $fail failed ==="
if [ "$fail" -gt 0 ]; then
echo "Failures:$FAILED_NAMES"
exit 1
fitest/lib/vc_oracle_common.sh:56-63
vc_oracle_assert_match() {
local name="$1" dl_out="$2" dt_out="$3"
if [ -z "$dl_out" ] && [ -z "$dt_out" ]; then
fail=$((fail+1))
FAILED_NAMES="$FAILED_NAMES $name"
echo " FAIL: $name (both sides empty — schema/function/vtable likely broke)"
return 1
fiEvidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Reject a silently empty sourced suite**
**What failed:** The empty-suite guard test expected a sourced suite with no executable oracle cases to fail, but the copied runner printed 0 passed, 0 failed and returned exit 0.
- **Impact:** An empty or missing sourced suite can report success without executing any cases, allowing regressions in the intended feature coverage to reach users unnoticed.
- **Steps to reproduce:**
1. Temporarily replace the five sourced feature-family files with empty files while leaving the runner unchanged.
2. Run test/vc_oracle_feature_interaction_test.sh with the normal DoltLite and Dolt executable arguments.
3. Observe the 0 passed, 0 failed summary and exit code 0, then restore the family files and confirm the normal run executes 752 cases.
- **Stub / mock content:** The test intentionally substituted temporary empty family sources to model missing coverage; no product mocks, route interceptions, or production-data bypasses were used.
- **Code analysis:** At line 11, pass and fail both start at zero. Lines 55-57 source the five family files, but sourcing an empty or incomplete fragment invokes no oracle and therefore never reaches vc_oracle_assert_match. Lines 60-64 print the counters and exit nonzero only when fail is greater than zero, so zero executed cases are treated as success. The helper's both-empty output check at test/lib/vc_oracle_common.sh:56-63 does not protect this path because it only runs when an oracle call exists. The smallest fix is to add a final pass + fail -gt 0 check before the existing fail check and return a nonzero status when no cases ran.
- **Why this is likely a bug:** The reproduced result is independent of the browser or external services and follows directly from the runner's control flow: removing all sourced assertions leaves both counters at zero, yet the success condition checks only fail. Because this runner is intended to prove that all five feature families remain covered after the PR's split, silently accepting zero cases defeats that safety check; adding an explicit nonzero-case guard is a targeted fix.
**Relevant code:**
`test/vc_oracle_feature_interaction_test.sh:11`
~~~bash
pass=0; fail=0
~~~
`test/vc_oracle_feature_interaction_test.sh:55-64`
~~~bash
for family in merge history schema query stress; do
source "$SCRIPT_DIR/lib/vc_oracle_feature_interactions/$family.sh"
done
echo ""
echo "=== Results: $pass passed, $fail failed ==="
if [ "$fail" -gt 0 ]; then
echo "Failures:$FAILED_NAMES"
exit 1
fi
~~~
`test/lib/vc_oracle_common.sh:56-63`
~~~bash
vc_oracle_assert_match() {
local name="$1" dl_out="$2" dt_out="$3"
if [ -z "$dl_out" ] && [ -z "$dt_out" ]; then
fail=$((fail+1))
FAILED_NAMES="$FAILED_NAMES $name"
echo " FAIL: $name (both sides empty — schema/function/vtable likely broke)"
return 1
fi
~~~
This was referenced Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TableEntryandSchemaEntryone canonical owner indoltlite_catalog_types.hinstead of duplicate guarded layouts in the VC and Prolly internalsTests
make -C build -j6 doltlite doltlite-remotesrv sqlite3.cbash test/vc_oracle_feature_interaction_test.sh build/doltlite dolt(752 passed)bash test/vc_oracle_merge_test.sh build/doltlite dolt(74 passed)bash test/vc_oracle_schema_merge_test.sh build/doltlite dolt(54 passed)bash test/run_doltlite_regression_case.sh all(309,956 passed)bash test/lint_layers.shbash test/lint_orphaned_suites.shbash test/dead_code_check.shbash -n test/vc_oracle_feature_interaction_test.sh test/lib/vc_oracle_feature_interactions/*.shTableEntryand oneSchemaEntrydefinitionmake -C build devtestreproduced the known unfiltered baseline (1,620 compatibility failures, then the final-file hang at 2,545/2,546; interrupted at 2m35s)