Fix jdbc-v2: report precision/scale of SimpleAggregateFunction-wrapped columns - #3043
Open
polyglotAI-bot wants to merge 1 commit into
Open
Fix jdbc-v2: report precision/scale of SimpleAggregateFunction-wrapped columns#3043polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
…d columns ResultSetMetaData.getPrecision()/getScale() read the outer ClickHouseColumn. SimpleAggregateFunction(func, T) parses into dataType=SimpleAggregateFunction with T kept as the nested column, and the wrapper's own data type declares no precision or scale, so both accessors returned 0 - losing the declared scale of DateTime64 and both precision and scale of Decimal. The wrapper is transparent on the read path, so the accessors now describe the nested column. AggregateFunction is left as is: its values are aggregation states, not values of the nested type. Fixes: #3042
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
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.
Description
Fixes #3042.
ResultSetMetaDataImpl.getPrecision(int)/getScale(int)read the outerClickHouseColumn. ASimpleAggregateFunction(func, T)type name parses intodataType = SimpleAggregateFunctionwith the real type keptas the nested column (
ClickHouseColumn.java:443-449), andupdate()then fills precision from the wrapper's owndata type (
column.precision = column.dataType.getMaxPrecision(), which is 0 for the wrapper) while theparameter-driven
Decimal/DateTime64branches never run. So both accessors returned 0 and the declaredprecision/scale of the wrapped type was lost. The wrapper is transparent on the read path — client-v2
BinaryStreamReader.readValuereads aSimpleAggregateFunctioncolumn by recursing intogetNestedColumns().get(0)— so the metadata that describes the values a caller reads must come from the nestedcolumn, which is what both accessors now do.
LowCardinalitywas already unaffected (the parser treats it as a flagrather than a nesting level, so
dataTypeis already the inner type).AggregateFunctionis deliberately not unwrapped: it is not transparent (its values are aggregation states, readvia
readBitmap), so the nested type's precision/scale does not describe them. That distinction is pinned by a testrow.
Changes
jdbc-v2/.../metadata/ResultSetMetaDataImpl.java:getPrecision/getScaleresolve the value-describing columnthrough a new private
valueColumn(ClickHouseColumn)helper that unwrapsSimpleAggregateFunction(and only that)to its nested column. No public API, no other accessor, and no other data type changes behavior.
CHANGELOG.md: bug-fix entry.Test
ResultSetMetaDataImplTest.testGetPrecisionAndScaleOfWrappedTypes— one TestNG@DataProvider-parametrizedintegration test (asserts through the real JDBC query path) covering:
SimpleAggregateFunction(any, Decimal(18, 4))→18/4,SimpleAggregateFunction(sum, Decimal(38, 10))→38/10SimpleAggregateFunction(any, Nullable(Decimal(18, 4)))→18/4SimpleAggregateFunction(any, DateTime('Europe/Amsterdam'))→29/0,SimpleAggregateFunction(any, DateTime64(3, 'Europe/Amsterdam'))→29/3SimpleAggregateFunction(any, FixedString(16))→16/0(precision from a non-numeric parameter)Decimal(18, 4)(18/4),LowCardinality(DateTime(tz))(29/0),SimpleAggregateFunction(groupArrayArray, Array(Decimal(18, 4)))(0/0,same as a plain
Arraycolumn — the unwrap does not dig past the first level), and anAggregateFunctionstatefrom
sumState(toDecimal64(1, 4))(0/0, unchanged)Verified in both directions: the 6
SimpleAggregateFunctionrows fail on unpatchedmain(each reporting0) andpass with the fix; the 4 contrast rows pass before and after. No existing test was modified.
Test runs (jdbc-v2, against ClickHouse 26.5.1):
mvn -pl jdbc-v2 test→ 1298 tests, 0 failuresmvn -pl jdbc-v2 -DskipUTs=true -Dit.test="ResultSetMetaDataImplTest,ResultSetImplTest,ParameterMetaDataImplTest,DatabaseMetaDataTest,JdbcDataTypeTests,JDBCDateTimeTests" verify→ 169 tests, 0 failuresCompatibility
Behavior change is limited to
getPrecision/getScaleforSimpleAggregateFunction-wrapped result columns, whichpreviously returned
0(no usable information). Nothing else is affected: no public API signature, no formattedoutput, no other metadata accessor, no other data type. Per
docs/changes_checklist.mdthis is a"conditional logic or guard changed" / metadata-value change — the boundary cases (empty nested list, non-SAF
wrapper,
AggregateFunction, container nested types) are covered above.docs/features.mdneeds no update:result-set metadata backed by the ClickHouse column schema is already listed, and this restores the documented
contract rather than adding or altering a feature.
Out of scope (recorded on the issue, not addressed here to keep one concern per PR):
getObject(int)without a typehint returns
ZonedDateTimeforSimpleAggregateFunction(any, DateTime(tz))butjava.sql.Timestampfor the plainand
LowCardinality-wrapped forms, becauseJdbcUtilsmapsSimpleAggregateFunctiontoJDBCType.OTHER. Changingthat alters an existing value's returned Java class, so it deserves its own decision.