[fix](jdbc) Resolve SQL Server user-defined alias types by JDBC type code - #67916
Merged
Merged
Conversation
…code Columns declared with a SQL Server alias type (CREATE TYPE ... FROM base_type) were mapped to UNSUPPORTED by the JDBC catalog, because DatabaseMetaData.getColumns() reports the alias name as TYPE_NAME and the SQL Server client only dispatched on that name. SELECT * on such a table then failed with "type UNSUPPORTED is unsupported for Nereids". The same result set still carries the base type in DATA_TYPE, COLUMN_SIZE and DECIMAL_DIGITS. Keep the name based mapping as the primary path and, only for names that are not SQL Server system types, resolve the column by its standard java.sql.Types code with a mapping that mirrors the name based one. Binary and vendor specific codes stay unsupported because mssql-jdbc reports CLR user-defined types as VARBINARY too. Applied to both JdbcSQLServerClient (fe-core) and JdbcSQLServerConnectorClient (fe-connector-jdbc). Unit tests cover alias columns of every base type family and the types that must remain unsupported. The SQL Server docker fixture gains alias types and a test_alias_type table which test_sqlserver_jdbc_catalog now checks with DESC and SELECT *. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#67916
commit: 39de21240f76b53ba3a5451c6a3f5070518205c3
base: 2573820600eb178150a35710a94fd58b478df96d
reviewed_at: 2026-09-14T00:15+08:00
reviewer: morningman
model: claude-opus-5
effort: max
findings: {blocker: 0, major: 0, minor: 2, nit: 2}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
…rted ones end to end Extend the SQL Server docker fixture so that test_sqlserver_jdbc_catalog exercises an alias over every supported base type family (character, integer, floating point, decimal and money, date and time, uniqueidentifier and sysname), an alias typed IDENTITY column, and a negative table whose aliases over binary types, datetimeoffset and sql_variant as well as the xml / CLR system types must stay UNSUPPORTED while the remaining columns are still readable and SELECT * keeps failing on them. The last table is also described through a catalog with enable.mapping.varbinary to show that the option does not change the alias behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16949 ms |
Contributor
TPC-DS: Total hot run time: 82129 ms |
Contributor
ClickBench: Total hot run time: 14.54 s |
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
Issue Number: close apache#67916 Related PR: apache#67917 Problem Summary: The max_ngram_diff creation limit incorrectly changed analyzer identity, allowing equivalent custom analyzers to bypass duplicate-index detection. FE also accepted non-ASCII digits that the BE integer parser rejects. Exclude max_ngram_diff from ngram tokenizer identity and require its value to use ASCII integer syntax. Release note: None Validation: - ./run-fe-ut.sh --run org.apache.doris.analysis.invertedindex.AnalyzerIdentityBuilderTest,org.apache.doris.indexpolicy.PolicyValidatorTests (27 tests passed) - FE Checkstyle passed as part of the targeted test run Behavior changed: Equivalent ngram analyzers now share an identity regardless of max_ngram_diff, and FE rejects non-ASCII max_ngram_diff values. Documentation impact: None
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: close apache#67916 Related PR: apache#67917 Problem Summary: An arbitrarily large max_ngram_diff could multiply token output without a hard fan-out bound, and the deterministic regression assertion was not stored as a runner-generated golden. Cap max_ngram_diff at 255 consistently in FE and BE, cover the accepted and rejected boundaries, and replace the manual token-list assertion with a named golden query generated by the regression runner. ### Release note The max_ngram_diff tokenizer setting accepts values from 0 through 255. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test - Behavior changed: - [ ] No. - [x] Yes. max_ngram_diff values above 255 are rejected to bound per-position token fan-out. - Does this need documentation? - [ ] No. - [x] Yes. Document the supported max_ngram_diff range. Validation: - ./build.sh --be -j8 (ASAN, Java extensions, build hygiene, and glibc compatibility) - ./build.sh --fe -j8 - NGramTokenizerTest: 16/16 passed - AnalyzerIdentityBuilderTest and PolicyValidatorTests: 29/29 passed - test_ngram_max_diff_custom_analyzer: runner-generated golden and clean comparison passed on an isolated local FE/BE - clang-format 16, clang-tidy, and Checkstyle passed
7 tasks
Contributor
FE Regression Coverage ReportIncrement line coverage |
morningman
pushed a commit
to morningman/doris-skills
that referenced
this pull request
Sep 14, 2026
…judge regressions against the merge base Review of apache#12 raised two gaps in the regression rule: 1. doc-templates.md scoped the Major floor to a list of categories while verify-review-docs.py floored every `Regression: yes` regardless, so a reviewer following the documented exception for an observability change produced a document the verifier rejected. 2. The regression evidence was `git show $BASE_SHA:<path>`, the target-branch tip. When the PR branch is behind that tip, a fix that landed on the target branch after the PR branched off is absent at HEAD without the PR having removed anything, and the rule would have turned it into a Major regression. Category-aware floor: - Every finding carries a mandatory `Category` / `类别` line from a closed vocabulary: functional-bug, functional-loss, data-error, resource-leak, performance, observability, test-coverage, wording, maintainability. The verifier rejects an unknown value (a typo must not escape the floor), accepts one parenthetical domain note after the class, resolves ZH aliases (功能性bug, 功能性缺失, 资源泄漏, 性能), and requires EN and ZH to agree. - A regression in the first five categories is at least Major. A regression in the other four may stay Minor/Nit only with a `Severity rationale` paragraph, which the verifier now checks for. - The JSON result adds `floored_regressions`. post-pass-comment.sh refuses on a floored regression (it can only appear if the two scripts disagree) and refuses to post when a non-floored regression would go undisclosed: the receipt needs at least one note, and SKILL.md / pr-comment-format.md say every such finding is named there first. Merge base: - Regression evidence is `git show $MERGE_BASE:<path>` in SKILL.md (premise checks, step 6), prompts.md (preamble, D1, E), the ledger skeletons and the templates; `{MERGE_BASE}` joins the prompt placeholders. `BASE_SHA` keeps its two jobs: the diff range and the commit the receipt binds to. - prepare-review-context.sh records `TARGET_AHEAD` (target-branch commits since the merge base) in meta.env and prints it when non-zero. - Two new common traps: filing a real behaviour change under a soft category, and comparing against the target-branch tip. Tests: verify/repo-review covers each floored category, the rationale requirement and acceptance for each non-floored category, the parenthetical note, spelling variants, missing / unknown / disagreeing categories, and the poster's disclosure gate. Suite: 63 PASS (was 47). Also replayed against the real apache/doris#67916 documents: they verify after relabelling with the closed vocabulary, a test-coverage regression is rejected without a rationale and accepted with one, and a functional-bug regression rated Minor is rejected; that review's own header already had BASE_SHA != MERGE_BASE (TARGET_AHEAD = 5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H2in7A8yA4qQ3LwAghueTR
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for one P1 correctness issue: the name-normalization path can bypass the new JDBC-code fallback for legal alias identifiers and declare the wrong FE schema.
Checkpoint conclusions:
- Goal/correctness and tests: ordinary underscore-named aliases resolve correctly, but the feature remains incorrect for legal aliases whose unqualified names are truncated to or collide with system names; current tests do not cover that case.
- Minimal scope: the eight-file change is focused, and the declared binary/vendor-code exclusions were respected during review.
- Concurrency and lifecycle: no new shared state, locking, background work, initialization, or cleanup behavior.
- Configuration: no new variable; existing
enable.mapping.varbinarybehavior remains scoped to the native-name path. - Compatibility and parallel paths: no persisted/protocol compatibility change. Both plugin and legacy metadata paths were traced through their callers and SQL Server scanners; both contain the same name-preemption defect, while their other added mappings and runtime object shapes agree.
- Conditionals: the new name-first/default-fallback branching is the identified failure point; the remaining supported/unsupported branches match the stated scope.
- Test coverage and results: the unit and external fixtures are otherwise internally consistent, deterministic, and cover both clients plus positive/negative families. This was a static review only; no builds or tests were run independently under the runner contract. Exact-head CI currently reports compile, CheckStyle, FE UT, and External Regression passing.
- Observability: no new service/runtime control path requiring metrics or logging; existing unsupported-type reporting is unchanged.
- Persistence, transactions, data writes, and FE/BE variables: none in production code; SQL writes are test fixtures only.
- Performance: only constant-size metadata dispatch is added; no material performance concern found.
- Other issues and user focus: no additional user focus was supplied, and no other distinct issue remained after two convergence rounds and duplicate fencing.
Overall: one distinct blocking finding at the exact reviewed head.
| // so that they never reach the JDBC type code fallback below. | ||
| return ConnectorType.of("UNSUPPORTED"); | ||
| default: | ||
| return jdbcTypeCodeToConnectorType(fieldInfo); |
Contributor
There was a problem hiding this comment.
[P1] Route legal alias identifiers through the base-code fallback
SQL Server permits alias type names to be delimited identifiers containing spaces or parentheses (CREATE TYPE, identifier rules). For CREATE TYPE dbo.[int alias] FROM varchar(50), getColumns() supplies TYPE_NAME = int alias and the base DATA_TYPE = VARCHAR; the normalization at line 58 reduces the name to int, so this fallback is never reached and FE exposes the VARCHAR column as INT. The legacy client has the same preemption. Please restrict IDENTITY normalization to a code-compatible IDENTITY form (or otherwise prefer unambiguous base codes before name dispatch) and cover legal alias names and system-name collisions in both paths.
…ame when its code agrees
An alias type may legally be named with spaces or parentheses:
CREATE TYPE dbo.[int alias] FROM varchar(50) is reported by getColumns()
with TYPE_NAME "int alias" and DATA_TYPE VARCHAR. Both SQL Server
clients cut the name at the first space or parenthesis to strip the
IDENTITY decoration ("int identity", "decimal(18,0) identity"), which
turned such an alias into the system type its name starts with and
declared the column as INT.
Strip the decoration only when the name has the IDENTITY form of one of
the base types IDENTITY is allowed on and DATA_TYPE is that base type's
code; any other name is matched as it is, so an alias name that is not a
system type name falls through to the JDBC type code as intended (an
alias cannot share a system type's name: SQL Server rejects it).
Unit tests cover such alias names and the IDENTITY forms the driver
versions report; the docker fixture gains dbo.test_alias_name with
[int alias], [decimal(18,0) identity] and [int identity] aliases next to
a real IDENTITY column.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16694 ms |
Contributor
TPC-DS: Total hot run time: 82256 ms |
Contributor
ClickBench: Total hot run time: 14.65 s |
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#67916
commit: 80fe649113ebee7203507478875676caaa39fc86
base: 2573820600eb178150a35710a94fd58b478df96d
reviewed_at: 2026-09-14T17:40+08:00
reviewer: morningman
model: claude-opus-5
effort: max
findings: {blocker: 0, major: 0, minor: 2, nit: 2}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
CalvinKirs
approved these changes
Sep 14, 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
3 participants
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.
What problem does this PR solve?
Issue Number: close #67793
Related PR: none
Problem Summary:
Columns declared with a SQL Server user-defined alias type (
CREATE TYPE dbo.customtexttype FROM varchar(50)) are mapped toUNSUPPORTED_TYPEby the JDBC catalog, soSELECT *on such a table fails withJdbcSQLServerClient.jdbcTypeToDoris()(and its counterpartJdbcSQLServerConnectorClient.jdbcTypeToConnectorType()infe-connector-jdbc) dispatches only onTYPE_NAME. For an alias type,DatabaseMetaData.getColumns()reports the alias name (customtexttype) asTYPE_NAME, so the name never matches and thedefaultbranch returnsUNSUPPORTED. The same result set however still carries the base type inDATA_TYPE(java.sql.Types.VARCHAR),COLUMN_SIZE(50) andDECIMAL_DIGITS.This PR keeps the name-based switch as the primary mapping (it carries SQL Server specific choices such as
tinyint -> SMALLINTandmoney -> DECIMAL(19,4)) and, only when the name is not recognised, resolves the column by its standardjava.sql.Typescode. The fallback mirrors the name-based mapping and is applied in both implementations:fe/fe-core/.../jdbc/client/JdbcSQLServerClient.javafe/fe-connector/fe-connector-jdbc/.../JdbcSQLServerConnectorClient.javaScope of the fallback (verified against the mssql-jdbc
DataTypeFilterthat post-processesgetColumns()):DATA_TYPEThe name-based path strips the IDENTITY decoration a column is reported with (
int identity,decimal() identity,numeric(18, 0) identity,decimal(18,0) IDENTITY(1,1)) only when the name has that form for one of the base types IDENTITY is allowed on andDATA_TYPEis that base type's code. Any other name is matched as it is: SQL Server allows an alias to be named with spaces or parentheses (CREATE TYPE dbo.[int alias] FROM varchar(50), reported asTYPE_NAME = int alias,DATA_TYPE = VARCHAR), and cutting the name at the first space or parenthesis, as both clients did, turned such an alias into the system type its name starts with (an INT column for a varchar alias). An alias cannot share a system type's name outright (CREATE TYPE dbo.[int] ...is rejected by SQL Server), so an unrecognised name is an alias and goes to the code fallback.Deliberately not resolved by the fallback:
BINARY,VARBINARY,LONGVARBINARY): mssql-jdbc reports CLR user-defined types (geometry,geography,hierarchyid, ...) asVARBINARYtoo, so an alias overvarbinarycannot be told apart from an unsupported CLR type by the type code alone. They stayUNSUPPORTED.sql_variant,datetimeoffsetaliases): stayUNSUPPORTED.CREATE TYPE dbo.[varbinary alias] FROM varbinary(20)) isUNSUPPORTEDas well. Before this PR its name was cut tovarbinaryand the column happened to be readable; now the name is not a system type name and the binary code is not resolved.xml,sql_variant,geometry,geography,hierarchyid,json,vectorare now listed explicitly as unsupported system types so that the fallback never changes their existing behavior (xmlwould otherwise be reported asLONGNVARCHAR).The BE side needs no change: the scanner reads values by the Doris column type (
getObject()/getBigDecimal()), and the driver returns the Java object of the base type for alias columns.Release note
Fix SQL Server JDBC catalog mapping columns of user-defined alias types (
CREATE TYPE ... FROM base_type) toUNSUPPORTED_TYPE; they are now resolved to the Doris type of their base type. Aliases overbinary,varbinary,image,datetimeoffsetandsql_variantremainUNSUPPORTED.Check List (For Author)
Test
Regression test
Unit Test
Manual test (add detailed scripts or steps below)
No need to test or manual test. Explain why:
Unit tests: new
JdbcSQLServerClientTest(fe-core) and extendedJdbcSQLServerConnectorClientTest(fe-connector-jdbc) cover alias columns for every base type family, prove that unknown / CLR / vendor-specific types stayUNSUPPORTED, that the name-based mapping still takes precedence, and that alias names which start with a system type name (int alias,decimal(18,0) identity,int identityover varchar) are resolved by their code while the real IDENTITY forms of every driver version still resolve by name.Regression test: the SQL Server docker fixture now creates aliases over every supported base type family (
dbo.test_alias_type, incl.sysname), an alias typed IDENTITY column (dbo.test_alias_identity) and a negative table (dbo.test_alias_unsupported: aliases over binary types / datetimeoffset / sql_variant plus xml / geometry / hierarchyid), anddbo.test_alias_namewith aliases named[int alias],[decimal(18,0) identity]and[int identity]next to a real IDENTITY column.test_sqlserver_jdbc_catalogassertsDESCandSELECT *on the positive tables, and for the negative table that the columns are reported asUNSUPPORTED_TYPE, that the other columns stay readable, thatSELECT *still fails, and thatenable.mapping.varbinarydoes not change the alias behavior. Verified end to end on a local 1 FE + 1 BE cluster built from this branch against the SQL Server 2022 docker image: the suite passes and the real output of the nine new blocks is identical to the committed.out. The suite is in groupp2, which the External Regression pipeline excludes (excludeGroups = "p1,p2"), so CI does not execute it; the local run above is the only execution. Note that the JDBC catalog runs through thefe-connector-jdbcplugin, so the regression test exercisesJdbcSQLServerConnectorClient; the fe-coreJdbcSQLServerClient(used by the streaming / CDC path) is covered by its unit test, and the two mappings are equivalent case by case.Metadata evidence:
sp_columns_100(ODBC v3, what mssql-jdbc calls fromgetColumns()) on SQL Server 2022 reports alias columns withTYPE_NAME= alias name andDATA_TYPE= the base type code: varchar 12, nvarchar -9, int 4, bigint -5, tinyint -6, bit -7, decimal and money 3 (money with precision 19 / scale 4), float 6 (the driver maps it toDOUBLE), date 91, datetime/datetime2/smalldatetime 93 with the base scale, time -154 (mapped toTIME), uniqueidentifier -11 (mapped toCHAR), datetimeoffset -155, binary/varbinary/image -2/-3/-4, sql_variant -150, xml -152 (mapped toLONGNVARCHAR), geometry/geography/hierarchyid -151 (mapped toVARBINARY).Behavior changed:
sysname) are now readable through the JDBC catalog instead of being reported asUNSUPPORTED_TYPE. An alias whose name starts with a system type name ([int alias]) is no longer mistaken for that system type: it is resolved by its base type, or reported asUNSUPPORTEDwhen the base type is one of the five families above. Columns of other SQL Server types keep their previous mapping.Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code