diff --git a/pkg/col/coldata/batch.go b/pkg/col/coldata/batch.go index 993707f0a18f..761d8a517be9 100644 --- a/pkg/col/coldata/batch.go +++ b/pkg/col/coldata/batch.go @@ -398,7 +398,17 @@ func (m *MemBatch) AppendCol(col *Vec) { // ReplaceCol implements the Batch interface. func (m *MemBatch) ReplaceCol(col *Vec, colIdx int) { - if m.b[colIdx] != nil && m.b[colIdx].t != nil && !m.b[colIdx].Type().Identical(col.Type()) { + // NB: we use Equivalent() rather than Identical() here because + // ReplaceCol is a physical-level operation - it swaps column vector + // pointers. What matters for correctness is that the replacement + // vector has the same physical representation (i.e. the same canonical + // type family), not that the logical SQL types are exactly identical. + // For example, VARBIT(1) and VARBIT (no width limit) are both backed + // by DatumVec and are physically interchangeable, but Identical() + // rejects them because they differ in Width. Equivalent() checks type + // family compatibility which is the right semantic here. + // Fixes: https://github.com/cockroachdb/cockroach/issues/172870 + if m.b[colIdx] != nil && m.b[colIdx].t != nil && !m.b[colIdx].Type().Equivalent(col.Type()) { panic(fmt.Sprintf("unexpected replacement: original vector is %s "+ "whereas the replacement is %s", m.b[colIdx].Type(), col.Type())) } @@ -409,11 +419,11 @@ func (m *MemBatch) ReplaceCol(col *Vec, colIdx int) { func (m *MemBatch) Reset(typs []*types.T, length int, factory ColumnFactory) { cannotReuse := m == nil || m.Capacity() < length || m.Width() < len(typs) for i := 0; i < len(typs) && !cannotReuse; i++ { - // TODO(yuzefovich): change this when DatumVec is introduced. - // TODO(yuzefovich): requiring that types are "identical" might be an - // overkill - the vectors could have the same physical representation - // but non-identical types. Think through this more. - if v := m.ColVec(i); !v.Type().Identical(typs[i]) { + // NB: we use Equivalent() rather than Identical() to determine batch + // reusability. Vectors with the same physical representation but + // non-identical logical types (e.g. VARBIT(1) vs VARBIT) should + // allow the batch to be reused. + if v := m.ColVec(i); !v.Type().Equivalent(typs[i]) { cannotReuse = true break } diff --git a/pkg/sql/logictest/testdata/logic_test/union b/pkg/sql/logictest/testdata/logic_test/union index da1185d9a240..55b7bdd3195c 100644 --- a/pkg/sql/logictest/testdata/logic_test/union +++ b/pkg/sql/logictest/testdata/logic_test/union @@ -799,3 +799,33 @@ TABLE t130591_1 INTERSECT TABLE t130591_1; statement count 1 TABLE t130591_1 UNION TABLE t130591_1; + +# Regression test for #172870. UNION ALL with BIT/VARBIT expressions that have +# width constraints should not cause vectorized execution internal errors. The +# bug was that ReplaceCol used Type().Identical() which rejected types like +# VARBIT(1) vs VARBIT (different Width) even though they have the same physical +# representation in the vectorized engine. + +query T +SELECT IF(false, ARRAY[]::VARBIT(1)[], NULL) GROUP BY 1 UNION ALL SELECT NULL; +---- +NULL +NULL + +query T +SELECT IF(false, ARRAY[]::BIT(1)[], NULL) GROUP BY 1 UNION ALL SELECT NULL; +---- +NULL +NULL + +query T +SELECT IF(false, B''::VARBIT(1), NULL) GROUP BY 1 UNION ALL SELECT NULL; +---- +NULL +NULL + +query T +SELECT IF(false, B''::BIT(1), NULL) GROUP BY 1 UNION ALL SELECT NULL; +---- +NULL +NULL