Skip to content

docs: fix Enum8/Enum16 type mappings in TYPES.md#1930

Open
sankalpsthakur wants to merge 2 commits into
ClickHouse:mainfrom
sankalpsthakur:fix/1901-types-md-enum-docs
Open

docs: fix Enum8/Enum16 type mappings in TYPES.md#1930
sankalpsthakur wants to merge 2 commits into
ClickHouse:mainfrom
sankalpsthakur:fix/1901-types-md-enum-docs

Conversation

@sankalpsthakur

@sankalpsthakur sankalpsthakur commented Jul 22, 2026

Copy link
Copy Markdown

Summary

Fixes #1901

Fixes incorrect Go type mappings for ClickHouse Enum8 / Enum16 in TYPES.md.

Enum8 was documented as converting to orb.Point (and other geo rows were similarly shifted), while lib/column/enum8.go / enum16.go actually map enums to string on scan and accept string / integer / fmt.Stringer on append.

Root cause

Enum8 and Enum16 columns were added to the table headers without inserting matching empty cells in data rows, so existing geo X marks landed two columns left (under the Enum columns).

Changes

  • Realign orb.Point / Ring / Polygon / MultiPolygon / LineString / MultiLineString marks under the correct ClickHouse type columns
  • Document Enum support matching the implementation:
    • Append: string, int, int8 (Enum8), int16 (Enum16), fmt.Stringer
    • Scan: string, sql.Scanner
  • Normalize row widths so all data rows have the full column count

Checklist

  • Documentation fix only (no code or tests required)

Enum8 and Enum16 were shown as mapping to orb.Point / orb.Ring
because the geo columns shifted when Enum headers were added
without updating row cells.

- Realign Point/Ring/Polygon/etc. X marks under the correct columns
- Document string (and int/int8/int16/fmt.Stringer for Append) support
  for Enum types, matching lib/column/enum{8,16}.go
- Mark sql.Scanner Scan support for Enum8/Enum16

Fixes ClickHouse#1901
@CLAassistant

CLAassistant commented Jul 22, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread TYPES.md
| int64 | | | | | | | | | | | | | | | X | | | | | | | | X | X | | | | | | | | | | |
| int32 | | | | | | | | | | | | | | X | | | | | | | | | | | | | | | | | | | | |
| int16 | | | | | | | | | | | | | X | | | | | | | | | | | | | X | | X | | | | | | |
| int8 | | | | | | | | | | | | X | | | | | | | | | | | | | X | | X | | | | | | | |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Should fix — Append table still marks int8/int16/string against Time/Time64

The Time/Time64 columns accept only time.Duration/*time.Duration (plus driver.Valuer) — see lib/column/time.go:117 (AppendRow) and :74 (Append). Appending an int8, int16, or string returns a ColumnConverterError, so these marks are wrong:

  • int8 → Time (this line) and int16 → Time64 (line 22) — the Scan-table rows correctly leave these blank, which confirms the Append marks are stray.
  • string → Time/Time64 (line 26).

These are pre-existing on main and likely the same misalignment #1901 fixes (the Time/Time64 columns were inserted without shifting data cells). The correct Enum8/Enum16 marks are already present on these rows, so just drop the Time/Time64 marks.

@github-actions

Copy link
Copy Markdown

🤖 Claude review

Docs-only PR realigning the two type-mapping tables in TYPES.md and adding Enum8/Enum16 coverage (fixes #1901).

The core fix is correct. I traced the pipe-delimited cell alignment of both tables. The geo rows (orb.Point/Ring/Polygon/MultiPolygon/LineString/MultiLineString) now land under their own ClickHouse columns instead of shifting two cells left onto Enum8/Enum16 — on main, orb.Point's mark renders under Enum8, which is exactly the #1901 symptom. The new Enum8/Enum16 marks match the implementation: append accepts string/int/int8/int16/fmt.Stringer and scan accepts string/sql.Scanner (lib/column/enum8.go, enum16.go).

One issue: the realignment preserves incorrect Time/Time64 marks in the Append table — int8 → Time, int16 → Time64, and string → Time/Time64 — but lib/column/time.go only accepts time.Duration/*time.Duration/driver.Valuer. These are pre-existing on main and look like the same header-insertion misalignment #1901 fixes (Time/Time64 were inserted without shifting data cells). Since this PR rewrites every row to match the implementation, they are worth correcting here. See inline.

Blind spots:

  • Verification was by manual pipe-cell counting (no python/awk available in this review environment). I validated the method against the known Ошибка в types.md для Enum #1901 symptom and roughly a dozen rows, but did not machine-check all 66 rows for exact width.
  • []byte → MultiPolygon is preserved from main (full-width, unchanged) and looks dubious, but I did not confirm its intended column, so it is not flagged.

Verdict: ⚠️ Request changes

Inline comments are attached to the relevant lines. This summary updates in place on re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ошибка в types.md для Enum

3 participants