fix(#142): address Copilot post-merge review on #317#318
Merged
Conversation
Two correctness improvements caught after #317 merged: 1. ``IndexingStage.process``: the AUTO-INDEXING startup log and the empty-range warning both printed the raw ``conf.AUTO_INDEX_THRESHOLD``, which is misleading when the operator set the sentinel ``-1`` (interpreted as ``record_count``). The log would say ``[3, -1]`` even though the effective upper bound was the row count, and the warning could read ``threshold (-1)`` when MIN exceeded the resolved-but-tiny record_count. Now both lines use a single ``threshold_display`` that shows the resolved value, annotated with the raw sentinel when relevant (e.g. ``12345 (-1 → record_count)``). Threshold resolution is hoisted above the log so operators see the effective range that will actually be applied. 2. ``config_declaration.yaml``: the ``auto_index_threshold`` description inverted Postgres indexing wisdom. It claimed "indexing higher-cardinality columns rarely helps because the Postgres planner won't pick the index over a sequential scan for selective queries" — but high-cardinality columns are *more* selective and B-tree indexes shine exactly there; the planner avoids indexes for *low*-selectivity predicates. Reworded to explain the real reason DP+ limits auto-indexing: the right high-cardinality index depends on actual query patterns, and blindly indexing every column on ingest inflates storage, slows COPY, and adds vacuum/bloat pressure for indexes the workload may never use. Existing ``test_issue_142_auto_index_threshold.py`` suite (8 tests) still passes — its warning-message assertion checks for the ``min_threshold`` / ``threshold`` substrings, both of which the updated message preserves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Two correctness improvements caught by Copilot's review of #317 after it merged. Both replies were posted on the original PR.
IndexingStage.process: AUTO-INDEXING log and empty-range warning printed the rawconf.AUTO_INDEX_THRESHOLD. When the operator used the-1sentinel ("no upper bound" →record_count), the log read[3, -1]and the warning could surfacethreshold (-1)— both misleading. Threshold resolution is now hoisted above the log; both lines usethreshold_display, which shows the resolved value annotated with the raw sentinel when relevant (e.g.12345 (-1 → record_count)).config_declaration.yaml: theauto_index_thresholddescription inverted Postgres indexing wisdom — it implied high-cardinality columns are bad index candidates because the planner skips selective queries. Actually the opposite: high-cardinality columns are more selective and B-tree indexes help there; the planner avoids indexes for low-selectivity predicates. Reworded to explain the real reason DP+ limits auto-indexing (right index depends on query patterns; blind ingest-time indexing inflates storage, slows COPY, adds vacuum pressure).Test plan
tests/test_issue_142_auto_index_threshold.py— all 8 tests pass; the warning-message assertion checks formin_threshold/thresholdsubstrings, both still presentAUTO_INDEX_THRESHOLD=-1and a small-record dataset to confirm the rendered text🤖 Generated with Claude Code