morningman commented on PR #66670:
URL: https://github.com/apache/doris/pull/66670#issuecomment-5475620558
## Findings
### F-01 · `length` is emitted for CHAR/VARCHAR but silently dropped for
VARBINARY
- **Severity**: Minor
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:119-122`
- **Category**: correctness / API completeness
```java
case CHAR:
case VARCHAR:
result.length = ((ScalarType) type).getLength();
break;
```
**What is wrong.** `VARBINARY` is the one other column type whose length
genuinely varies per column,
and it is not in this arm, so a `VARBINARY(64)` column reports no `length`
at all while a
`VARCHAR(64)` column reports `"length": 64`.
**Why it happens.** `VARBINARY` is a supported column type —
`fe/fe-type/src/main/java/org/apache/doris/catalog/PrimitiveType.java:55`
declares
`VARBINARY("VARBINARY", 16, TPrimitiveType.VARBINARY, true)` — and it
carries a per-column length:
`fe/fe-type/src/main/java/org/apache/doris/catalog/ScalarType.java:154`
dispatches
`case VARBINARY: return createVarbinaryType(len)`, and `ScalarType.toSql`
renders `varbinary(N)`.
The `switch` in `fromType` is an allow-list, so `VARBINARY` falls to
`default:` and only `kind` and
`sql` survive.
**When it bites.** A MySQL JDBC catalog with `enable.mapping.varbinary=true`
maps a remote
`VARBINARY(64)` through `createVarbinaryType`; a client reading `type_desc`
for that column gets
`{"kind":"VARBINARY","sql":"varbinary(64)"}` and must fall back to parsing
the SQL string — the
exact failure mode issue #66675 set out to remove. The class's own contract
at
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:51-52`
says
clients should prefer the structured fields.
**Suggested fix.**
```diff
case CHAR:
case VARCHAR:
+ case VARBINARY:
result.length = ((ScalarType) type).getLength();
break;
```
### F-02 · VARIANT and AGG_STATE keep their nesting in `sql` but get no
structure in `type_desc`
- **Severity**: Minor
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:128-129`
- **Category**: API completeness
```java
default:
break;
}
return result;
```
**What is wrong.** `VARIANT` and `AGG_STATE` are the two remaining column
types whose `toSql()`
string carries real nested type structure, and both fall through to
`default:`, so the structured
view of a structured type contains no structure at all.
**Why it happens.** `VariantType.toSql` emits
`variant<<predefined fields...>,PROPERTIES ("variant_max_subcolumns_count" =
"...", ...)>` — the
predefined sub-fields are ordinary Doris types with their own names and
types, and they are dropped.
`AggStateType.toSql` emits `agg_state<fn(t1 null, t2 not null)>` from its
`subTypes` and
`subTypeNullables` lists, also dropped. Neither has a case in the switch.
**When it bites.** A client reading a `VARIANT` column with predefined
fields gets
`{"kind":"VARIANT","sql":"variant<...PROPERTIES
(\"variant_max_subcolumns_count\" = \"0\", ...)>"}`.
It cannot use the structured view, and the `sql` fallback is hostile to a
naive parser because the
`PROPERTIES (...)` clause of internal storage tuning knobs is interleaved
with the field list.
**Suggested fix.** Either add a `fields` arm for
`VariantType.getPredefinedFields()` (reusing
`StructFieldDesc`) and a `sub_types` arm for `AggStateType`, or — if that is
deliberately out of
scope for v1 — say so in the class javadoc so a client knows the omission is
intentional rather than
an oversight. The former is preferable; the whole point of the field is to
stop clients parsing SQL
text.
### F-03 · `type_sql` can contain the non-SQL sentence `unknown type:
UNSUPPORTED_TYPE`
- **Severity**: Minor
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java:87`
- **Category**: correctness / contract
```java
columnInfo.put("type_sql", colType.toSql());
```
**What is wrong.** `type_sql` is documented as "Complete SQL representation
intended for display and
compatibility fallback"
(`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:51-52`),
but for
a column of an unmapped external type it emits an English diagnostic
sentence, not SQL.
**Why it happens.** `ScalarType.toSql`'s final arm is
`default: stringBuilder.append("unknown type: ").append(type);`
(`fe/fe-type/src/main/java/org/apache/doris/catalog/ScalarType.java:733-735`),
and
`fe/fe-type/src/main/java/org/apache/doris/catalog/PrimitiveType.java:33`
declares `UNSUPPORTED("UNSUPPORTED_TYPE", -1, TPrimitiveType.UNSUPPORTED,
false)`,
so the rendered string is literally `unknown type: UNSUPPORTED_TYPE`. It
lands in both `type_sql`
and `type_desc.sql`.
**When it bites.** A MySQL JDBC catalog table containing a `GEOMETRY` column:
`JdbcMySQLClient.jdbcTypeToDoris` falls to `default: return
Type.UNSUPPORTED;`
(`fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcMySQLClient.java:357-358`),
and
`JdbcClient.getColumnsFromJdbc`
(`fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcClient.java:411-421`)
adds
**every** field to the schema without filtering unsupported ones. `GET
/api/{jdbc_catalog}/{db}/{tbl}/_schema`
then returns `"type_sql": "unknown type: UNSUPPORTED_TYPE"`. Nothing crashes
— `Type.UNSUPPORTED` is a
`ScalarType`, so the switch's `default:` handles it — but a client that
treats `type_sql` as SQL text
gets a sentence.
**Suggested fix.** Omit `type_sql`/`sql` when the primitive type is
`UNSUPPORTED`, so `@JsonInclude(NON_NULL)`
drops the key and the client sees only `kind: "UNSUPPORTED_TYPE"` — which is
unambiguous and already
matches the pre-existing `type` field. Alternatively narrow the javadoc to
admit the case.
### F-04 · `type_desc` drops the STRUCT field comment that `sql` keeps
- **Severity**: Minor
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:143-146`
- **Category**: API completeness
```java
public static class StructFieldDesc {
private final String name;
private final boolean containsNull;
private final SchemaTypeDesc type;
```
**What is wrong.** A STRUCT field's comment survives into the `sql` string
but not into the
structured view, so for STRUCT types `type_desc` is strictly lossier than
the string it is supposed
to replace.
**Why it happens.** `StructField.toSql` appends `comment '<c>'` when a
comment is set —
`fe/fe-type/src/main/java/org/apache/doris/catalog/StructField.java:112-114`
— while `StructFieldDesc`
captures only `name`, `containsNull` and `type`.
**When it bites.** `STRUCT<a: INT COMMENT 'unit price'>` yields
`sql: "struct<a:int comment 'unit price'>"` next to
`fields: [{"name":"a","contains_null":true,"type":{...}}]`. A client that
follows the class's own
advice at
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:51-52`
and prefers the structured fields loses the comment; one that
wants it must parse the SQL string after all.
**Suggested fix.**
```diff
public static class StructFieldDesc {
private final String name;
private final boolean containsNull;
+ private final String comment;
private final SchemaTypeDesc type;
private StructFieldDesc(StructField field) {
this.name = field.getName();
this.containsNull = field.getContainsNull();
+ this.comment = field.isCommentSpecified() ? field.getComment()
: null;
this.type = SchemaTypeDesc.fromType(field.getType());
}
```
`@JsonInclude(NON_NULL)` on the class already omits the key when no comment
is set.
### F-05 · No test drives a complex type through the real HTTP stack, and
the wire format is unpinned
- **Severity**: Minor
- **Where**:
`fe/fe-core/src/test/java/org/apache/doris/http/TableSchemaActionTest.java:51-54`
- **Category**: test coverage
```java
JSONObject column = (JSONObject) propArray.get(0);
Assert.assertEquals("BIGINT", column.get("type"));
Assert.assertEquals("bigint", column.get("type_sql"));
Assert.assertEquals("BIGINT", ((JSONObject)
column.get("type_desc")).get("kind"));
```
**What is wrong.** Complex types are the entire point of the feature, yet no
test sends an
ARRAY/MAP/STRUCT column through the endpoint. Worse, the one assertion that
does exercise the real
serialization stack cannot detect a wire-format regression: a BIGINT node
renders as the
byte-identical `{"kind":..., "sql":...}` under Jackson-with-annotations,
Jackson-without-`@JsonNaming`,
and Gson alike. The snake_case keys that actually matter — `contains_null`,
`key_contains_null`,
`value_contains_null`, `element`, `fields` — appear only in unit tests that
construct their own
`new ObjectMapper()`, which proves nothing about what Spring Boot's
converter emits.
**Why it happens.** The only HTTP-level test uses the shared fixture, whose
columns are just BIGINT
and DOUBLE
(`fe/fe-core/src/test/java/org/apache/doris/http/DorisHttpTestCase.java:139-140`),
and the
existing end-to-end suite for this exact endpoint,
`regression-test/suites/http_rest_api/get/test_schema_api.groovy:64-65`,
still asserts only
`resultList.size() == 6` and never reads `type_sql` or `type_desc`. The PR's
own checklist leaves
"Regression test" unchecked, while root `AGENTS.md` says to prioritise
regression tests under
`regression-test/`.
**When it bites.** If `@JsonNaming` were dropped in a later refactor, or
Spring's converter
configuration changed, every key of the new object would silently become
camelCase and CI would stay
green.
**Suggested fix.** Extend `test_schema_api.groovy` — not the shared FE
fixture. Adding a
complex-typed column to `DorisHttpTestCase` would break two unrelated tests
that assert the fixture
is exactly `[k1, k2]`
(`fe/fe-core/src/test/java/org/apache/doris/datasource/doris/RemoteDorisCompatibleRestClientTest.java:53-59`
and
`fe/fe-core/src/test/java/org/apache/doris/datasource/doris/RemoteDorisRestClientTest.java:81-87`).
The groovy suite already creates its own table: add `ARRAY<DECIMAL(18,4)>`,
a `MAP`, and a `STRUCT`
column and assert the snake_case keys and nested values on the live response.
### F-06 · `assertFalse(node.path(key).asBoolean())` passes when the key is
absent
- **Severity**: Minor
- **Where**:
`fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/response/SchemaTypeDescTest.java:129`
- **Category**: test coverage
```java
JsonNode map = eventsField.path("type").path("element");
Assert.assertFalse(map.path("key_contains_null").asBoolean());
Assert.assertTrue(map.path("value_contains_null").asBoolean());
```
**What is wrong.** `JsonNode.path()` returns a `MissingNode` for an absent
key and
`MissingNode.asBoolean()` returns `false`, so this assertion — and the
matching one at
`fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/response/SchemaTypeDescTest.java:135`
— passes
whether the key is present-and-`false` or missing entirely. The negative
half of the nullability
contract is therefore not pinned: if `key_contains_null` stopped being
serialized, the test would
still be green.
**Why it happens.** `assertFalse(...asBoolean())` conflates "false" with
"absent". The correct idiom
is already used in the same file at
`fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/response/SchemaTypeDescTest.java:172-173`,
which uses `json.has(...)`.
**Suggested fix.**
```diff
- Assert.assertFalse(map.path("key_contains_null").asBoolean());
+ Assert.assertTrue(map.has("key_contains_null"));
+ Assert.assertFalse(map.get("key_contains_null").asBoolean());
```
Apply the same change at line 135 for `contains_null`.
### F-07 · A user-visible public API change ships with `Release note: None`
- **Severity**: Minor
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java:87-88`
- **Category**: compatibility / process
```java
columnInfo.put("type_sql", colType.toSql());
columnInfo.put("type_desc", SchemaTypeDesc.fromType(colType));
```
**What is wrong.** The PR description ticks "Behavior changed: **Yes** — The
table schema API
response includes the additive `type_sql` and `type_desc` fields" and
simultaneously writes
"Release note: **None**", and ticks "Does this need documentation? **No**"
for two new fields on a
public REST API.
**Why it happens.** Root `AGENTS.md`, Commit Standards rule 4: "The `Release
note` section must be
filled in for any user-visible behavior or feature change; write 'None' for
internal refactoring or
test-only changes." This is a `[feature]` PR on a public endpoint, so the
note is required. The
commit message itself is only the title plus `Signed-off-by:`, with none of
the PR-template body.
**When it bites.** The release note is the only channel that tells operators
the response shape
grew. It also matters more here than for the endpoint's previous additive
changes (#46557 added
nullable info, #56919 added `column_uid`/`schema_version`): those added
string values, whereas this
is the first time a value under `properties[]` is a nested **object**. A
consumer that tolerates
unknown keys is unaffected, but one that models a column as
`Map<String,String>` breaks on an object
value where every previous addition was survivable.
**Suggested fix.** Replace "None" with a note naming the two fields and
stating that they are
additive and that existing fields are unchanged, and open the corresponding
`doris-website` PR for
the schema API page.
### F-08 · ARRAY `contains_null` is a compile-time constant, and three
assertions on it can never fail
- **Severity**: Nit
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:60-63`
- **Category**: API design / test coverage
```java
// ARRAY attributes. containsNull describes element nullability, and
element is the recursively
// described item type.
private Boolean containsNull;
private SchemaTypeDesc element;
```
**What is wrong.** For ARRAY nodes this is advertised as a per-column
attribute but can never be
anything but `true`, and the tests that assert it are tautologies.
**Why it happens.** `ArrayType.getContainsNull()` has the body `return
true;` with the javadoc
"Always returns `true`. Array elements are always nullable in Doris."
(`fe/fe-type/src/main/java/org/apache/doris/catalog/ArrayType.java:78-83`),
and the two-argument
constructor is `@Deprecated` and explicitly discards its `containsNull`
argument. The consequence in
the tests is that
`fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/response/SchemaTypeDescTest.java:108-110`
are three assertions on a literal, and the test name
`testNestedArraysReportNullableElementsAtEveryLevel` overclaims — only its
fourth assertion, on
`kind`, can fail. This is not wrong output (Doris array elements really are
always nullable), but a
client cannot tell that ARRAY's flag is constant while MAP's genuinely
varies.
**Suggested fix.** Note in the comment at
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:60-61`
that ARRAY `contains_null` is
always `true` in Doris and is emitted for symmetry with MAP, and rename the
test to reflect that it
checks nested `element` chaining rather than nullability.
### F-09 · The response repeats the same SQL text at every level, and
`type_sql` duplicates `type_desc.sql`
- **Severity**: Nit
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java:87-88`
- **Category**: performance
```java
columnInfo.put("type_sql", colType.toSql());
columnInfo.put("type_desc", SchemaTypeDesc.fromType(colType));
```
**What is wrong.** `type_sql` is byte-identical to `type_desc.sql`, and
because every node renders
its own subtree from depth 0
(`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/response/SchemaTypeDesc.java:76`,
`this.sql = type.toSql();`), a type of depth
*d* ships its leaf's SQL text *d* times. Measured duplication for realistic
nested types is 5–7×.
**Why it happens.** `buildColumnInfo` runs for every base column **and** for
every column of every
materialized index
(`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java:162-167`),
and `getIndexIdToMeta()` re-emits the base index as well.
**When it bites.** Per column the addition is roughly +65 B for BIGINT, +111
B for
`DECIMALV3(18,4)`, and +609 B for `ARRAY<STRUCT<3 fields>>`. For a
500-column table with three
100-column rollups (1300 `buildColumnInfo` calls) the body grows from ~155
KiB to ~237 KiB
(+53%) when all columns are scalar, and to ~928 KiB (+499%) when all are
complex. The CPU cost is
O(n·d) with *d* bounded by `Type.MAX_NESTING_DEPTH` = 9
(`fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java:53`), so this
is a payload concern, not
a CPU one — and it is paid by every caller, including
`RemoteDorisRestClient.isTableExist()`, which downloads the whole body and
reads only `code`.
**Suggested fix.** Optional, and a deliberate trade: issue #66675 explicitly
asked for both fields,
so keeping `type_sql` is defensible as a convenience for clients that never
descend into
`type_desc`. If payload matters more, drop `type_sql` and let clients read
`type_desc.sql`, or emit
`sql` only on the root node.
### F-10 · `buildColumnInfo` was widened for a test without
`@VisibleForTesting`
- **Severity**: Nit
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java:74`
- **Category**: style
```java
static Map<String, Object> buildColumnInfo(Column column) {
```
**What is wrong.** The method went from `private` to package-private
`static` solely so
`TableSchemaActionColumnInfoTest` (which lives in the same package) can call
it, but nothing records
that intent. 113 files under `fe/fe-core/src/main` use `@VisibleForTesting`
for exactly this.
**Suggested fix.** Add `@VisibleForTesting` above the declaration. The
widening itself is sound — the
new test needs same-package access and the alternative is testing only
through HTTP.
### F-11 · New standalone test files use JUnit 4
- **Severity**: Nit
- **Where**:
`fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/response/SchemaTypeDescTest.java:30-31`
- **Category**: style
```java
import org.junit.Assert;
import org.junit.Test;
```
**What is wrong.** Part 4.3 of the repo's `code-review` skill asks for JUnit
5, and `fe-core` is now
majority JUnit 5 (910 files vs 410). The `httpv2` tree is mixed, but the
newest file in the exact
target package,
`fe/fe-core/src/test/java/org/apache/doris/httpv2/rest/TSOActionTest.java`, is
JUnit 5.
**Suggested fix.** Move the two **new standalone** files
(`SchemaTypeDescTest`, `TableSchemaActionColumnInfoTest`) to JUnit 5. Leave
`fe/fe-core/src/test/java/org/apache/doris/http/TableSchemaActionTest.java`
on JUnit 4 — it extends
the JUnit-4 `DorisHttpTestCase` and cannot be converted in isolation.
### F-12 · The one in-tree consumer of this endpoint is not wired up
(pre-existing, informational)
- **Severity**: Nit
- **Where**:
`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/TableSchemaAction.java:88`
- **Category**: observation — **not a defect of this PR**
```java
columnInfo.put("type_desc", SchemaTypeDesc.fromType(colType));
```
**What is wrong.** Nothing, in this PR. But it is worth telling the author
that Doris Catalog
federation already reads this endpoint and reads it wrongly, and that this
PR builds precisely the
metadata that would fix it.
**Why it happens.** `RemoteDorisCompatibleRestClient.getColumns` calls
`api/{db}/{tbl}/_schema`
(`fe/fe-core/src/main/java/org/apache/doris/datasource/doris/RemoteDorisCompatibleRestClient.java:50`),
and `parseColumn`
(`fe/fe-core/src/main/java/org/apache/doris/datasource/doris/RemoteDorisCompatibleRestClient.java:79-111`)
reads keys `nullable`, `key` and `type_attributes` that this endpoint has
never emitted — it emits
`is_nullable` (`"Yes"`/`"No"`), `is_key`, and flat top-level
`precision`/`scale`. So nullability and
key-ness always come back `false`, and precision/scale are never applied. It
then calls
`ScalarType.createType(typeName)`, which for `"ARRAY"`/`"MAP"`/`"STRUCT"`
hits
`default: LOG.warn(...); Preconditions.checkState(false);` in
`fe/fe-type/src/main/java/org/apache/doris/catalog/ScalarType.java` and
throws.
**When it bites.** Federating a remote Doris table with any ARRAY/MAP/STRUCT
column throws today,
before and after this PR.
**Suggested fix.** Out of scope here — but worth a follow-up issue, and
`type_desc` is the natural
input for that fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
