repeat_row: Wrangle negative accumulation error messages#36229
Draft
ggevay wants to merge 1 commit intoMaterializeInc:mainfrom
Draft
repeat_row: Wrangle negative accumulation error messages#36229ggevay wants to merge 1 commit intoMaterializeInc:mainfrom
repeat_row: Wrangle negative accumulation error messages#36229ggevay wants to merge 1 commit intoMaterializeInc:mainfrom
Conversation
ggevay
changed the title
repeat_row: Wrangle negative accumulation error messages
Replace the scattered "internal error: ..." messages raised on
negative/non-positive/non-monotonic multiplicities and inconsistent
accumulators with a single structured EvalError variant,
MultiplicityError { kind, code_place, detail }.
The new Display does not use the "internal error:" prefix; it names the
likely root cause (invalid data in a Materialize source, caused either
by a bug in the external system feeding the source or by a bug in
Materialize) and mentions incorrect usage of the repeat_row table
function as a secondary cause.
Migrated sites:
* src/compute/src/render/reduce.rs (DistinctBy, ReduceInaccumulable,
ReduceMinsMaxes, MinsMaxesHierarchical, hierarchical mins-maxes final,
ReduceMonotonic, AccumulableErrorCheck net-zero + unsigned-sum).
* src/compute/src/render/top_k.rs (MonotonicTopK, TopK validator,
MonotonicTop1).
* src/compute/src/compute_state.rs (persist peek, index peek error
trace) and compute_state/peek_result_iterator.rs (index peek ok
trace), using err.to_string() to feed PeekResponse::Error.
* src/adapter/src/coord/peek.rs and src/adapter/src/peek_client.rs
constant-result fast paths: emit EvalError::MultiplicityError instead
of a hand-rolled InvalidParameterValue / AdapterError::Unstructured.
* src/storage-operators/src/s3_oneshot_sink.rs: build and stringify
a MultiplicityError, mirroring the [customer-data] logging contract
inline (storage-operators cannot depend on compute).
* src/expr/src/relation/func.rs (scalar-subquery cardinality check):
the old EvalError::NegativeRowsFromSubquery variant is removed.
Internal error reporting is unified: every MultiplicityError site logs
through log_multiplicity_error (free function in render/errors.rs, and
a thin forwarding method on ErrorLogger). All sites share the static
title "invalid record multiplicity", so they merge into a single Sentry
group; kind/code_place/detail go into a [customer-data]-tagged WARN
breadcrumb.
Additionally fix the reduce.rs monoid-variant mismatch site: the lhs/rhs
Row values no longer leak into the panic/Sentry title; a static title
helps with Sentry's issue merging.
Proto backcompat is not needed for EvalError::NegativeRowsFromSubquery
because it was introduced this week and hasn't shipped in any release;
its proto tag (83) is reused by the new ProtoMultiplicityError.
Co-authored-by: Junie <junie@jetbrains.com>
ggevay
force-pushed
the
repeat_row-8-error-msgs
branch
from
8ac5c2a to
7b1c538
Compare
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.
DRAFT
Resolves SQL-165.
TODO:
Replace the scattered "internal error: ..." messages raised on negative/non-positive/non-monotonic multiplicities and inconsistent accumulators with a single structured EvalError variant, MultiplicityError { kind, code_place, detail }.
The new Display does not use the "internal error:" prefix; it names the likely root cause (invalid data in a Materialize source, caused either by a bug in the external system feeding the source or by a bug in Materialize) and mentions incorrect usage of the repeat_row table function as a secondary cause.
Migrated sites:
Internal error reporting is unified: every MultiplicityError site logs through log_multiplicity_error (free function in render/errors.rs, and a thin forwarding method on ErrorLogger). All sites share the static title "invalid record multiplicity", so they merge into a single Sentry group; kind/code_place/detail go into a [customer-data]-tagged WARN breadcrumb.
Additionally fix the reduce.rs monoid-variant mismatch site: the lhs/rhs Row values no longer leak into the panic/Sentry title; a static title helps with Sentry's issue merging.
Proto backcompat is not needed for EvalError::NegativeRowsFromSubquery because it was introduced this week and hasn't shipped in any release; its proto tag (83) is reused by the new ProtoMultiplicityError.