Repository navigation
chore(deck): upgrade @geoarrow/deck.gl-layers to @geoarrow/[email protected] - #745
Conversation
Signed-off-by: Ihor Dykhta <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR switches the deck package to ChangesGeoArrow deck.json migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Ihor Dykhta <[email protected]>
This reverts commit af6836d.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08dbd2453b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const table = data as arrow.Table; | ||
| const batches = table.batches; |
There was a problem hiding this comment.
When a managed dataset is still loading, createDeckJsonConfiguration returns data: [] for GeoArrow layers; this adapter treats any truthy value as an Arrow Table and immediately reads table.batches. On the initial/loading render, batches is undefined and batches.length throws instead of rendering an empty layer, so maps can crash until the dataset becomes ready. Guard non-Table values before this split.
Useful? React with 👍 / 👎.
| batchProps[propName] = chunkData; | ||
| } | ||
| } else { | ||
| batchProps[propName] = propValue; |
There was a problem hiding this comment.
For multi-batch tables, otherProps still contains the parent id, so this assignment overwrites the per-batch id created above with the same id for every sublayer. Deck tracks layers by id, so multiple RecordBatches from one table produce duplicate GeoArrow layer ids and can cause later batches to replace or confuse earlier ones during updates/picking/rendering; exclude id from otherProps or apply the generated id last.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/deck/README.md (1)
20-20: 📐 Maintainability & Code Quality | 🔵 TrivialUpdate the GitHub repository URLs to match the renamed repository.
The repository for
@geoarrow/deck.gl-geoarrowhas been renamed fromdeck.gl-layerstodeck.gl-geoarrow. While the existing link redirects successfully, it should be updated to the canonical URL to align with the package name and eliminate reliance on redirects.
Location Current URL packages/deck/README.md:20https://github.com/geoarrow/deck.gl-layerspackages/deck/README.md:442https://github.com/geoarrow/deck.gl-layersUpdate both instances to:
https://github.com/geoarrow/deck.gl-geoarrow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/README.md` at line 20, The README links for `@geoarrow/deck.gl-geoarrow` still point to the old deck.gl-layers repository URL; update both GitHub links in the packages/deck/README.md references to use the canonical deck.gl-geoarrow repository so they match the package name and avoid redirect reliance.packages/deck/src/json/layers/createTableToRecordBatchAdapter.ts (1)
14-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the new adapter behavior.
This adapter encapsulates non-trivial branching (RecordBatch passthrough, multi-batch splitting, Vector→Data mapping). A small test over a 2-batch
Tableand aRecordBatchinput would lock in the contract.As per coding guidelines: "Add or update tests when changing behavior or public API, when practical".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/src/json/layers/createTableToRecordBatchAdapter.ts` around lines 14 - 71, Add unit tests for createTableToRecordBatchAdapter to lock in the new branching behavior: verify that a RecordBatch input is passed through to a single UpstreamLayer instance unchanged, and that a multi-batch arrow.Table is split into one sublayer per batch with the expected batch-specific data and props mapping. Use the createTableToRecordBatchAdapter factory and the Adapter/renderLayers path in the test so the contract stays covered if internals change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/deck/src/json/layers/createTableToRecordBatchAdapter.ts`:
- Around line 29-36: The RecordBatch passthrough in
createTableToRecordBatchAdapter currently forwards otherProps without converting
arrow.Vector values to arrow.Data, which can break downstream layers expecting
Data. Reuse the same Vector-to-Data conversion logic already applied in the
Table branch, and run it in the RecordBatch path before constructing the
UpstreamLayer so rewriteGeoArrowAccessors outputs are normalized consistently
for both branches.
In `@packages/deck/src/json/layers/DeckColumnLayer.ts`:
- Around line 29-39: The DeckColumnLayer export currently only wraps
GeoArrowColumnLayer with createTableToRecordBatchAdapter, but the wireframe
workaround for deck.gl `#10021` is missing while the comment still claims it
exists. Either reintroduce the ColumnLayer.prototype.draw patch around
GeoArrowColumnLayer before exporting DeckColumnLayer, or confirm the underlying
`@deck.gl/layers` version is already at the fixed v9.3.4+ and then update the
comment to remove the obsolete workaround reference.
---
Nitpick comments:
In `@packages/deck/README.md`:
- Line 20: The README links for `@geoarrow/deck.gl-geoarrow` still point to the
old deck.gl-layers repository URL; update both GitHub links in the
packages/deck/README.md references to use the canonical deck.gl-geoarrow
repository so they match the package name and avoid redirect reliance.
In `@packages/deck/src/json/layers/createTableToRecordBatchAdapter.ts`:
- Around line 14-71: Add unit tests for createTableToRecordBatchAdapter to lock
in the new branching behavior: verify that a RecordBatch input is passed through
to a single UpstreamLayer instance unchanged, and that a multi-batch arrow.Table
is split into one sublayer per batch with the expected batch-specific data and
props mapping. Use the createTableToRecordBatchAdapter factory and the
Adapter/renderLayers path in the test so the contract stays covered if internals
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9ab005f-ae26-4dd1-b2ed-cdbf8830133c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/deck/README.mdpackages/deck/package.jsonpackages/deck/src/json/compileGeoArrowAccessor.tspackages/deck/src/json/defaultClasses.tspackages/deck/src/json/layers/DeckColumnLayer.tspackages/deck/src/json/layers/DeckH3HexagonLayer.tspackages/deck/src/json/layers/DeckTripsLayer.tspackages/deck/src/json/layers/createTableToRecordBatchAdapter.tspackages/deck/src/json/layers/index.tspackages/deck/src/json/rewriteGeoArrowAccessors.tspackages/deck/src/prepare/types.ts
| * GeoArrowColumnLayer adapter that accepts Table + Vector data (the format | ||
| * used by our JSON pipeline) and bridges to the 0.4.x RecordBatch API. | ||
| * Also includes a fix for the deck.gl ColumnLayer wireframe index buffer | ||
| * bug (#10021). | ||
| * | ||
| * Remove this wrapper once deck.gl includes the upstream fix (PR #10094). | ||
| * Remove the prototype patch once deck.gl includes the upstream fix (PR #10094). | ||
| */ | ||
| export class DeckColumnLayer< | ||
| ExtraProps extends object = object, | ||
| > extends GeoArrowColumnLayer<ExtraProps> { | ||
| static layerName = 'GeoArrowColumnLayer'; | ||
| } | ||
| export const DeckColumnLayer = createTableToRecordBatchAdapter( | ||
| GeoArrowColumnLayer as any, | ||
| 'GeoArrowColumnLayer', | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect what the previous DeckColumnLayer implementation did (the removed prototype patch)
fd -t f 'DeckColumnLayer.ts' packages/deck
git log --oneline -n 5 -- packages/deck/src/json/layers/DeckColumnLayer.ts
git show HEAD~1:packages/deck/src/json/layers/DeckColumnLayer.ts 2>/dev/null | sed -n '1,80p'
# Check whether the adapter implements any wireframe / prototype patch
rg -nP -C3 'wireframe|prototype|10021|10094' packages/deck/srcRepository: sqlrooms/sqlrooms
Length of output: 2556
🌐 Web query:
deck.gl PR #10094 merged ColumnLayer wireframe index buffer fix version
💡 Result:
The fix for the ColumnLayer wireframe index buffer issue was implemented in deck.gl Pull Request #10094, titled "fix(ColumnLayer): closed shapes with binary data and transition props" [1]. This pull request was merged into the master branch and officially included in deck.gl version v9.3.4 [2][3]. The fix addresses a bug where the ColumnLayer, which used a shared geometry for both fill and wireframe models, would incorrectly re-attach wireframe indices to the fill model during binary-data transitions or HMR (Hot Module Replacement) rebuilds [1]. This resulted in "open" or missing faces in the rendered columns [1]. The PR resolved this by ensuring the fill model uses a separate, non-indexed geometry [1].
Citations:
- 1: fix(ColumnLayer): closed shapes with binary data and transition props visgl/deck.gl#10094
- 2: https://github.com/visgl/deck.gl/releases/tag/v9.3.4
- 3: https://npmx.dev/package-changelog/@deck.gl/layers/v/9.3.4
🌐 Web query:
@geoarrow/deck.gl-geoarrow changelog 0.4 0.4.1 0.4.2 ColumnLayer wireframe fix
💡 Result:
The @geoarrow/deck.gl-geoarrow package (formerly @geoarrow/deck.gl-layers) versions 0.4.0, 0.4.1, and 0.4.2 do not contain specific mentions of a ColumnLayer wireframe fix in their respective release notes [1][2][3][4][5]. The release v0.4.1 (published 2026-05-18) included only the following changes: - Exporting types from types.ts through the top-level package [1]. - A fix for extending objects [1]. While a significant fix for ColumnLayer wireframe/rendering issues (specifically related to closed shapes and binary data) was merged into the upstream deck.gl library in March 2026 (Pull Request #10094), there is no evidence that @geoarrow/deck.gl-geoarrow addressed a similar issue in the 0.4.x release series [1][6]. Users experiencing issues with ColumnLayer wireframe rendering are likely encountering behavior inherent to the core deck.gl library rather than the geoarrow-specific wrapper [7][6].
Citations:
- 1: https://github.com/geoarrow/deck.gl-geoarrow/releases/tag/v0.4.1
- 2: https://github.com/geoarrow/deck.gl-geoarrow/releases
- 3: chore: Bump to 0.4.0 geoarrow/deck.gl-geoarrow#206
- 4: geoarrow/deck.gl-geoarrow@bd68af2
- 5: geoarrow/deck.gl-geoarrow@a4f4a7a
- 6: fix(ColumnLayer): closed shapes with binary data and transition props visgl/deck.gl#10094
- 7: https://deck.gl/docs/api-reference/layers/column-layer
Wireframe fix (deck.gl #10021) was removed but the comment falsely claims it remains.
The current code exports only createTableToRecordBatchAdapter(GeoArrowColumnLayer, ...) with no prototype patch or wireframe-related logic. The previous implementation explicitly overrode ColumnLayer.prototype.draw (lines 10–27 in HEAD~1) to work around deck.gl bug #10021; this workaround is entirely missing. The comment (lines 29–35) is stale and misleading.
The upstream fix exists in deck.gl v9.3.4 (PR #10094), but @geoarrow/deck.gl-geoarrow 0.4.x release notes do not indicate inheritance of this fix. Until the underlying ColumnLayer used by GeoArrowColumnLayer is confirmed to include v9.3.4+, the workaround must be re-applied inside or around the adapter.
Either:
- Re-apply the prototype patch within the adapter or in a wrapper before export, or
- Verify the
@deck.gl/layersversion beneath@geoarrow/deck.gl-geoarrowis ≥9.3.4 and update the comment to remove the obsolete prototype-patch reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/deck/src/json/layers/DeckColumnLayer.ts` around lines 29 - 39, The
DeckColumnLayer export currently only wraps GeoArrowColumnLayer with
createTableToRecordBatchAdapter, but the wireframe workaround for deck.gl `#10021`
is missing while the comment still claims it exists. Either reintroduce the
ColumnLayer.prototype.draw patch around GeoArrowColumnLayer before exporting
DeckColumnLayer, or confirm the underlying `@deck.gl/layers` version is already at
the fixed v9.3.4+ and then update the comment to remove the obsolete workaround
reference.
Signed-off-by: Ihor Dykhta <[email protected]>
Signed-off-by: Ihor Dykhta <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a7df932fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return new UpstreamLayer({ | ||
| ...otherProps, | ||
| id: `${this.props.id}-0`, | ||
| data, | ||
| } as Record<string, unknown>); |
There was a problem hiding this comment.
When a caller uses this RecordBatch branch with accessors from the batch (for example getPosition: recordBatch.getChild('geom')), otherProps still contains Arrow Vectors. The Table path converts vectors to Data chunks, but this branch sends them directly to the 0.4 GeoArrow layer, whose render path expects arrow.Data and throws for geometry accessors instead of rendering. Convert Vector props to their first chunk here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
looks like this is a real issue we should fix
Signed-off-by: Ihor Dykhta <[email protected]>
Signed-off-by: Ihor Dykhta <[email protected]>
|
Maybe for a follow-up PR:
Yes, it could make sense, but I would not change the whole prepared/map boundary just to remove Cleanest shape, to me:
{
table,
geometryColumn: arrow.Vector,
}expose something like: {
table,
batches: Array<{
batch: arrow.RecordBatch;
geometry: arrow.Data;
columns: Record<string, arrow.Data>;
rowOffset: number;
}>
}or more minimally, a helper: forEachGeoArrowBatch(layerData, (batchContext) => ...)That makes the batch split a first-class prepared/render concept without making all of SQLRooms batch-native.
{"@@type": "GeoArrowScatterplotLayer", "_sqlroomsBinding": {"dataset": "x"}}but internally SQLRooms can produce per-batch upstream layers using real So my answer: yes, but the boundary should become “table-first plus GeoArrow batch views,” not “everything becomes RecordBatch/Data.” That keeps SQLRooms’ local-first/query-first model intact while removing the fake constructor compatibility layer. Short-term, I’d keep the adapter and fix its |
Signed-off-by: Ihor Dykhta <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 162367f043
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const batches = table.batches; | ||
| const batchOffsets = new Int32Array(batches.length); |
There was a problem hiding this comment.
When a managed H3 layer is still loading, createDeckJsonConfiguration returns stripped props with data: [] and can leave an explicit getHexagon such as @@=h3 in place. This new batch-offset setup assumes data is an Arrow Table and immediately reads table.batches.length, so the initial/loading render crashes before the dataset becomes ready; guard non-Table placeholder data here as the generic GeoArrow adapter does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/deck/src/ai.ts (1)
190-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new
fitToDatacontract and normalization path.This change both narrows the accepted schema and silently rewrites malformed nested input, so a small test matrix for flat point/geometry/H3 cases plus the nested-wrapper repair would pay off quickly. As per coding guidelines,
**/*.{js,ts,jsx,tsx}: Add or update tests when changing behavior or public API, when practical.Also applies to: 274-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/deck/src/ai.ts` around lines 190 - 215, Add regression tests for the updated fitToData API and its normalization behavior. Cover the accepted fitToData shapes in ai.ts for flat point data (longitudeColumn/latitudeColumn), geometryColumn, and h3Column, and add a case for the nested-wrapper repair path that rewrites malformed input into the normalized contract. Update or add tests around the fitToData schema/normalization logic so the new public behavior is locked in.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/deck/__tests__/createTableToRecordBatchAdapter.test.ts`:
- Around line 141-150: This test is mislabeled because table.slice(0, 0) still
produces one empty RecordBatch, so it never covers the zero-batch branch in
createTableToRecordBatchAdapter. Update the case in
createTableToRecordBatchAdapter.test.ts to either build a true zero-batch Table
and assert callRenderLayers returns null for the !batches.length path, or rename
the test to reflect the single-empty-batch behavior. Use the existing
callRenderLayers helper and the adapter behavior in
createTableToRecordBatchAdapter as the reference points.
In `@packages/deck/src/ai.ts`:
- Around line 190-215: The fitToData schema description is missing the valid H3
dataset-only path, so update the description on the fitToData z.object in ai.ts
to mention that callers can provide just dataset when bounds can be inferred
from H3 layers via h3Column, alongside the existing geometryColumn and
longitudeColumn+latitudeColumn cases. Keep the wording aligned with
useDeckMapFitToBounds and the fitToData validator so the AI is guided toward the
supported H3 flow.
---
Nitpick comments:
In `@packages/deck/src/ai.ts`:
- Around line 190-215: Add regression tests for the updated fitToData API and
its normalization behavior. Cover the accepted fitToData shapes in ai.ts for
flat point data (longitudeColumn/latitudeColumn), geometryColumn, and h3Column,
and add a case for the nested-wrapper repair path that rewrites malformed input
into the normalized contract. Update or add tests around the fitToData
schema/normalization logic so the new public behavior is locked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 293525f0-d2c2-4212-b5a5-b98c2d9d5c86
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/deck/README.mdpackages/deck/__tests__/createTableToRecordBatchAdapter.test.tspackages/deck/src/ai.tspackages/deck/src/json/layers/createTableToRecordBatchAdapter.ts
✅ Files skipped from review due to trivial changes (1)
- packages/deck/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/deck/src/json/layers/createTableToRecordBatchAdapter.ts
| fitToData: z | ||
| .object({ | ||
| dataset: z.string().describe('Dataset id to compute bounds from.'), | ||
| longitudeColumn: z | ||
| .string() | ||
| .optional() | ||
| .describe('Longitude column name for point data.'), | ||
| latitudeColumn: z | ||
| .string() | ||
| .optional() | ||
| .describe('Latitude column name for point data.'), | ||
| geometryColumn: z | ||
| .string() | ||
| .optional() | ||
| .describe('WKB geometry column name for computing bounds.'), | ||
| h3Column: z | ||
| .string() | ||
| .optional() | ||
| .describe('H3 hex index column for computing bounds.'), | ||
| padding: z.number().optional(), | ||
| maxZoom: z.number().optional(), | ||
| }) | ||
| .optional() | ||
| .describe( | ||
| 'Fit map view to data bounds. Provide dataset plus either geometryColumn (for WKB geometry) or longitudeColumn+latitudeColumn (for separate coordinate columns). Example: {"dataset": "myDataset", "geometryColumn": "geom"}', | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the H3 dataset-only case in this schema description.
The validator allows { "dataset": "..." }, and useDeckMapFitToBounds can infer h3Column from bound layers, but the current description says callers need geometryColumn or longitudeColumn+latitudeColumn. That mismatch will push the AI away from the valid H3 path documented earlier in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/deck/src/ai.ts` around lines 190 - 215, The fitToData schema
description is missing the valid H3 dataset-only path, so update the description
on the fitToData z.object in ai.ts to mention that callers can provide just
dataset when bounds can be inferred from H3 layers via h3Column, alongside the
existing geometryColumn and longitudeColumn+latitudeColumn cases. Keep the
wording aligned with useDeckMapFitToBounds and the fitToData validator so the AI
is guided toward the supported H3 flow.
Signed-off-by: Ihor Dykhta <[email protected]>
Summary
@geoarrow/[email protected]with its successor@geoarrow/[email protected]createTableToRecordBatchAdapterutility to bridge the API change (0.4.x expectsRecordBatch+Datainstead ofTable+Vector)What changed
The 0.4.x package changed its layer input contract from
data: arrow.Tablewith geometry asarrow.Vectortodata: arrow.RecordBatchwith geometry asarrow.Data. The adapter splits a Table into per-batch sublayers and converts Vector props to their corresponding Data chunks — the same iteration the old library did internally. No data is copied; all geometry buffers are shared by reference.Summary by CodeRabbit