fix: tabular - #10
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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:
📝 WalkthroughWalkthroughThe PR adds station dataset access through ChangesStation dataset access
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant inspectStations
participant DClimateClient
participant StationsClient
participant StationDataset
Operator->>inspectStations: provide CID and query options
inspectStations->>DClimateClient: create client with gateway
DClimateClient->>StationsClient: access cached stations client
inspectStations->>StationsClient: load station dataset
StationsClient->>StationDataset: open station dataset through gateway
inspectStations->>StationDataset: apply selections and request records
StationDataset-->>inspectStations: return plan and records
inspectStations-->>Operator: print inspection results
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| "@dclimate/jaxray": "^0.7.0", | ||
| "@opentelemetry/api": "^1.9.1" | ||
| "@opentelemetry/api": "^1.9.1", | ||
| "@dclimate/dparquet": "file:../dparquet", |
There was a problem hiding this comment.
HIGH
@dclimate/dparquet points outside the repository, and that package in turn points to ../ipld-index. A normal CI checkout or npm consumer will not have either sibling directory, so npm install/npm ci and the published package fail to resolve dependencies. Use a published version or include these packages in an in-repository workspace.
| const hotDays = await stations | ||
| .nearest(29.98, -95.36) | ||
| .timeRange({ start: "2025-01-01", end: "2025-12-31" }) | ||
| .where({ element: "TMAX", op: "gt", value: 3500 }) // hundredths of °C |
There was a problem hiding this comment.
LOW
The example labels GHCND TMAX as hundredths of °C and filters with 3500, but the inspection script documents the preserved NOAA scale as tenths (317 = 31.7 °C). This query therefore means 350 °C and will normally return nothing; use 350 and document tenths consistently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@README.md`:
- Around line 82-83: Update the README StationsClient.load example to use a
complete valid CID, or explicitly mark the current value as a non-runnable
placeholder so readers do not copy an invalid request.
- Around line 110-118: Update the README section describing where(...) and
fragment-statistics pruning to clearly qualify it as an external or
backend-owned optimization rather than an SDK/client-side guarantee. Avoid
presenting .where(...).rows() and client.stations.load() behavior as implemented
unless those client paths actually provide it; limit the documentation to
behavior supported by the current code.
In `@scripts/inspect-stations.ts`:
- Around line 90-99: Update argument validation in the --limit and --near
parsing cases: require limit to be a finite integer, and require latitude and
longitude to fall within [-90, 90] and [-180, 180] respectively. Preserve the
existing invalid-argument error behavior and near-coordinate assignment for
valid inputs.
- Around line 129-135: Update the selector handling around args.near and
args.stations so passing both --near and --station is rejected explicitly before
either selection path runs. Preserve the existing nearest behavior when only
args.near is provided and station-ID selection when only args.stations is
provided, with a clear user-facing error for the conflicting combination.
🪄 Autofix
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 Plus
Run ID: 85b9b96f-42e5-434c-82b6-bed1440f6e7c
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
README.mdpackage.jsonscripts/inspect-stations.tssrc/client.tssrc/index.tssrc/stations/index.tssrc/stations/stations-client.tstests/stations.test.ts
| ```typescript | ||
| const stations = await client.stations.load({ cid: "bafyr4i..." }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a valid CID in the loading example.
StationsClient.load parses request.cid with CID.parse. The "bafyr4i..." value is invalid, so this example throws DatasetNotFoundError when copied. Use a full valid CID or mark the value as a non-runnable placeholder.
Proposed documentation fix
-const stations = await client.stations.load({ cid: "bafyr4i..." });
+// Replace with a full, valid station dataset root CID.
+const stations = await client.stations.load({ cid: "<full-root-cid>" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```typescript | |
| const stations = await client.stations.load({ cid: "bafyr4i..." }); | |
| // Replace with a full, valid station dataset root CID. | |
| const stations = await client.stations.load({ cid: "<full-root-cid>" }); |
🤖 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 `@README.md` around lines 82 - 83, Update the README StationsClient.load
example to use a complete valid CID, or explicitly mark the current value as a
non-runnable placeholder so readers do not copy an invalid request.
| case "--limit": args.limit = Number(value()); break; | ||
| case "--plan": args.plan = true; break; | ||
| case "--near": { | ||
| const parts = value().split(","); | ||
| const lat = Number(parts[0]); | ||
| const lon = Number(parts[1]); | ||
| if (parts.length !== 2 || !Number.isFinite(lat) || !Number.isFinite(lon)) { | ||
| throw new Error("--near expects <lat,lon>, e.g. --near 40.78,-73.97"); | ||
| } | ||
| args.near = [lat, lon]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate numeric arguments against their documented domains.
Fractional --limit values pass validation but slice truncates them. Coordinates outside valid latitude and longitude bounds also pass validation. Reject non-integer limits and reject latitude values outside [-90, 90] or longitude values outside [-180, 180].
Proposed fix
- case "--limit": args.limit = Number(value()); break;
+ case "--limit": {
+ const limit = Number(value());
+ if (!Number.isInteger(limit) || limit < 0) {
+ throw new Error("--limit must be a non-negative integer");
+ }
+ args.limit = limit;
+ break;
+ }
case "--plan": args.plan = true; break;
case "--near": {
const parts = value().split(",");
const lat = Number(parts[0]);
const lon = Number(parts[1]);
- if (parts.length !== 2 || !Number.isFinite(lat) || !Number.isFinite(lon)) {
+ if (
+ parts.length !== 2 ||
+ !Number.isFinite(lat) ||
+ !Number.isFinite(lon) ||
+ lat < -90 || lat > 90 ||
+ lon < -180 || lon > 180
+ ) {
throw new Error("--near expects <lat,lon>, e.g. --near 40.78,-73.97");
}Also applies to: 109-112
🤖 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 `@scripts/inspect-stations.ts` around lines 90 - 99, Update argument validation
in the --limit and --near parsing cases: require limit to be a finite integer,
and require latitude and longitude to fall within [-90, 90] and [-180, 180]
respectively. Preserve the existing invalid-argument error behavior and
near-coordinate assignment for valid inputs.
| "@dclimate/jaxray": "^0.7.0", | ||
| "@opentelemetry/api": "^1.9.1" | ||
| "@opentelemetry/api": "^1.9.1", | ||
| "@dclimate/dparquet": "file:../dparquet", |
There was a problem hiding this comment.
HIGH
file:../dparquet depends on a sibling directory outside this repository. Fresh clones/CI cannot install it, and a published package will retain an unusable local-path dependency for consumers. Publish @dclimate/dparquet and reference a registry version (or include it in a committed workspace).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
scripts/inspect-stations.ts (5)
149-159: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winSkip the full index walk for a closed date range.
When both
--fromand--toare present, Lines 157-158 already provide both bounds, but Line 153 still callslistStations()and computes unused extrema. This adds the documented large station-index read cost without changing the query. Only walk the index when one bound is missing.🤖 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 `@scripts/inspect-stations.ts` around lines 149 - 159, Update the date-range handling around selected.timeRange so dataset.listStations() and the earliest/latest calculations run only when args.from or args.to is missing. For a closed range with both bounds present, pass args.from and args.to directly without the full index walk; preserve the existing open-ended fallback behavior.
224-228: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the unit message element-specific.
The message is printed for every single-element query, but
--elementaccepts arbitrary element names. NOAA defines TMAX and TMIN as tenths of degrees Celsius, while SNOW and SNWD use millimeters and other elements use different units or scales. (ncei.noaa.gov) Restrict the message to known tenths-scaled elements or derive the unit from element metadata.🤖 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 `@scripts/inspect-stations.ts` around lines 224 - 228, Update the unit message in the single-element query flow around the element check so it is printed only for NOAA elements known to use tenths of degrees Celsius, such as TMAX and TMIN. Do not display the TMAX-specific message for arbitrary element names; alternatively, derive and report the unit from the element metadata.Source: MCP tools
98-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty
--nearcomponents.
Number("")returns0. Therefore,--near ,-73.97and--near 40.78,become valid inputs. Check both trimmed components before conversion.Proposed validation
const parts = value().split(","); + if (parts.length !== 2 || parts.some((part) => part.trim() === "")) { + throw new Error("--near expects <lat,lon>, e.g. --near 40.78,-73.97"); + } const lat = Number(parts[0]); const lon = Number(parts[1]); - if (parts.length !== 2 || !Number.isFinite(lat) || !Number.isFinite(lon)) { + if (!Number.isFinite(lat) || !Number.isFinite(lon)) {🤖 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 `@scripts/inspect-stations.ts` around lines 98 - 100, Update the --near parsing around value() so both comma-separated components are trimmed and validated as non-empty before converting them with Number. Reject inputs with a missing latitude or longitude, such as "--near ,-73.97" or "--near 40.78,", while preserving normal coordinate parsing.
205-212: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBound record retrieval before applying
--limit. The station API exposestoRecordsas an array-producing method. Line 207 therefore materializes all matching rows before line 212 applies the limit. Broad queries can fetch and retain every row even when the CLI prints only 10. Add bounded or streaming retrieval to@dclimate/tabular, or reject broad unbounded queries.🤖 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 `@scripts/inspect-stations.ts` around lines 205 - 212, Update the record retrieval flow around selected.toRecords so --limit bounds work before materializing results, using a bounded or streaming `@dclimate/tabular` API if available. Preserve the current output and elapsed-time reporting, and reject broad queries when no bounded retrieval path exists rather than loading all matching records.
167-169: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSet
process.exitCodeinstead of forcing an immediate exit.When output is piped,
process.exit()can terminate Node beforeconsole.log()orconsole.error()finishes. This can truncate usage and error text. Apply this to both exit paths.Proposed fix
console.log(USAGE); - process.exit(argv.length === 0 ? 1 : 0); + process.exitCode = argv.length === 0 ? 1 : 0; + return; ... - process.exit(1); + process.exitCode = 1;🤖 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 `@scripts/inspect-stations.ts` around lines 167 - 169, Replace the immediate process.exit calls in the argument handling block with process.exitCode assignments, preserving exit code 1 for missing arguments and 0 for help requests so output can flush before termination.Source: MCP tools
🤖 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 `@scripts/inspect-stations.ts`:
- Around line 153-155: Update the extrema calculation after
dataset.listStations() in the station inspection flow to avoid spreading covered
into Math.min or Math.max. Use a loop or reduce over the station records to
compute the earliest start and latest end while preserving the current
timestamp-based results.
---
Outside diff comments:
In `@scripts/inspect-stations.ts`:
- Around line 149-159: Update the date-range handling around selected.timeRange
so dataset.listStations() and the earliest/latest calculations run only when
args.from or args.to is missing. For a closed range with both bounds present,
pass args.from and args.to directly without the full index walk; preserve the
existing open-ended fallback behavior.
- Around line 224-228: Update the unit message in the single-element query flow
around the element check so it is printed only for NOAA elements known to use
tenths of degrees Celsius, such as TMAX and TMIN. Do not display the
TMAX-specific message for arbitrary element names; alternatively, derive and
report the unit from the element metadata.
- Around line 98-100: Update the --near parsing around value() so both
comma-separated components are trimmed and validated as non-empty before
converting them with Number. Reject inputs with a missing latitude or longitude,
such as "--near ,-73.97" or "--near 40.78,", while preserving normal coordinate
parsing.
- Around line 205-212: Update the record retrieval flow around
selected.toRecords so --limit bounds work before materializing results, using a
bounded or streaming `@dclimate/tabular` API if available. Preserve the current
output and elapsed-time reporting, and reject broad queries when no bounded
retrieval path exists rather than loading all matching records.
- Around line 167-169: Replace the immediate process.exit calls in the argument
handling block with process.exitCode assignments, preserving exit code 1 for
missing arguments and 0 for help requests so output can flush before
termination.
🪄 Autofix
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 Plus
Run ID: eef79228-28f9-4986-91f1-0dbacc059aa3
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.jsonscripts/inspect-stations.tssrc/stations/index.tssrc/stations/stations-client.tstests/stations.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- tests/stations.test.ts
- src/stations/stations-client.ts
- src/stations/index.ts
| "@dclimate/jaxray": "^0.7.0", | ||
| "@opentelemetry/api": "^1.9.1" | ||
| "@opentelemetry/api": "^1.9.1", | ||
| "@dclimate/tabular": "file:../dparquet", |
There was a problem hiding this comment.
HIGH
file:../dparquet points outside this repository. Fresh clones and CI cannot install it without that sibling directory, and a published package leaves consumers with an unresolved local dependency. Use a published @dclimate/tabular version or a committed workspace package.
| // Either bound alone is meaningful, so the missing side widens to the | ||
| // dataset's own extent rather than forcing the caller to pass both. | ||
| // Widening costs a full index walk, so only the open-ended side pays for it. | ||
| const covered = await dataset.listStations(); |
There was a problem hiding this comment.
MEDIUM
listStations() runs whenever either time bound is present, including when both --from and --to were supplied and no coverage lookup is needed. For GHCND this adds roughly 136k gateway reads to every bounded query; only walk the index when a bound is actually missing.
| // dataset's own extent rather than forcing the caller to pass both. | ||
| // Widening costs a full index walk, so only the open-ended side pays for it. | ||
| const covered = await dataset.listStations(); | ||
| const earliest = Math.min(...covered.map((s) => s.start.getTime())); |
There was a problem hiding this comment.
MEDIUM
Spreading the complete station index into Math.min passes one argument per station. With the documented ~136k GHCND stations this can exceed V8's argument/stack limit, making one-sided time queries throw after the index walk. Compute extrema iteratively instead.
| ```typescript | ||
| const stations = await client.stations.load({ cid: "bafyr4i..." }); | ||
|
|
||
| // Every station, with position and coverage window. |
There was a problem hiding this comment.
LOW
Station enumeration is asynchronous via await stations.listStations() (as used by the inspector); stations.stations does not provide the StationInfo collection shown here. This example therefore fails to type-check or iterate correctly.
|
|
||
| ```typescript | ||
| const hotDays = await stations | ||
| .nearest(29.98, -95.36) |
There was a problem hiding this comment.
LOW
nearest() returns a promise, so .timeRange() is invoked on that promise before the outer await applies. Await stations.nearest(...) first, then chain the synchronous selections.
| "@dclimate/jaxray": "^0.7.0", | ||
| "@opentelemetry/api": "^1.9.1" | ||
| "@opentelemetry/api": "^1.9.1", | ||
| "@dclimate/tabular": "file:../dparquet", |
There was a problem hiding this comment.
HIGH
file:../dparquet requires an untracked sibling directory, so npm ci from a fresh checkout and installation of the published package will fail to resolve @dclimate/tabular. Publish/reference a registry version or include it in a declared workspace.
| gatewayUrl: request.gatewayUrl ?? this.options.gatewayUrl, | ||
| ...(this.options.fetch ? { fetch: this.options.fetch } : {}), | ||
| }); | ||
| return StationDataset.open(source, root); |
There was a problem hiding this comment.
MEDIUM
Returning the raw StationDataset means errors from the documented chained API (select, timeRange, plan, rows, etc.) never pass through translateStationError; only StationsClient.nearest() translates them. Consequently callers still cannot reliably catch station query failures as DClimateClientError. Wrap the returned query surface or translate errors at every public execution boundary.
| // dataset's own extent rather than forcing the caller to pass both. | ||
| // Widening costs a full index walk, so only the open-ended side pays for it. | ||
| const covered = await dataset.listStations(); | ||
| const earliest = Math.min(...covered.map((s) => s.start.getTime())); |
There was a problem hiding this comment.
MEDIUM
Spreading every station timestamp into Math.min/Math.max can exceed V8's argument limit. The script itself identifies GHCND as roughly 136k stations, enough to trigger RangeError: Maximum call stack size exceeded when either time bound is omitted. Compute extrema with a loop or reduce.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/stations/stations-client.ts`:
- Line 62: Update nearest and its selection validation so any request with
within requires a non-empty columns array before load is called. Throw
InvalidSelectionError for missing or empty columns, and ensure the invalid path
does not forward withinRange or issue a gateway request.
🪄 Autofix
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 Plus
Run ID: 5cfab2a8-7a1a-4cb9-b5a6-21eb706aff1e
📒 Files selected for processing (3)
scripts/inspect-stations.tssrc/stations/stations-client.tstests/stations.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/stations.test.ts
- scripts/inspect-stations.ts
| * Resolution is per fragment (whole years, for GHCND), so a range landing | ||
| * anywhere in a year that has the column matches it. | ||
| */ | ||
| within?: { start: Date | string | number; end: Date | string | number }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
sed -n '1,180p' src/stations/stations-client.ts
printf '%s\n' '--- station-related files ---'
git ls-files | grep -E '(^|/)(stations|station)' | head -80
printf '%s\n' '--- references to withinRange and requireColumns ---'
rg -n -C 4 'withinRange|requireColumns|within.*columns|InvalidSelectionError' . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage'
printf '%s\n' '--- relevant tests and package metadata ---'
find . -maxdepth 3 -type f \( -iname '*station*test*' -o -iname '*station*spec*' -o -name 'package.json' \) -printRepository: dClimate/dclimate-client-js
Length of output: 44497
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,180p' src/stations/stations-client.ts
rg -n -C 5 'withinRange|requireColumns|within.*columns|InvalidSelectionError' . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage'
find . -maxdepth 4 -type f \( -iname '*station*test*' -o -iname '*station*spec*' \) -printRepository: dClimate/dclimate-client-js
Length of output: 45312
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- reader package files ---'
find node_modules/@dclimate/tabular -maxdepth 4 -type f -print | head -100
printf '%s\n' '--- reader contract and implementation ---'
rg -n -C 8 'findNearestStation|withinRange|requireColumns' node_modules/@dclimate/tabular \
-g '!*.map'
printf '%s\n' '--- station test implementation ---'
sed -n '1,245p' tests/stations.test.ts
printf '%s\n' '--- package version ---'
node - <<'JS'
const p = require('./node_modules/@dclimate/tabular/package.json');
console.log(JSON.stringify({name: p.name, version: p.version}, null, 2));
JSRepository: dClimate/dclimate-client-js
Length of output: 436
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 8 'findNearestStation|withinRange|requireColumns' node_modules/@dclimate/tabular \
-g '!*.map'
sed -n '1,245p' tests/stations.test.ts
node - <<'JS'
const p = require('./node_modules/@dclimate/tabular/package.json');
console.log(JSON.stringify({name: p.name, version: p.version}, null, 2));
JSRepository: dClimate/dclimate-client-js
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- dependency declarations ---'
cat package.json
find . -maxdepth 2 -type f \( -name 'package-lock.json' -o -name 'npm-shrinkwrap.json' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' \) -print
printf '%s\n' '--- station tests ---'
sed -n '1,245p' tests/stations.test.ts
printf '%s\n' '--- cited upstream test ---'
curl -fsSL \
https://raw.githubusercontent.com/dClimate/dclimate-client-js/dparquet/tests/stations.test.ts \
| rg -n -C 10 'withinRange|requireColumns|within.*columns|findNearestStation'
printf '%s\n' '--- upstream reader references ---'
git ls-remote https://github.com/dClimate/dclimate-tabular.git 'refs/heads/*' 'refs/tags/*' 2>/dev/null | head -40 || trueRepository: dClimate/dclimate-client-js
Length of output: 12669
🏁 Script executed:
#!/bin/bash
set -eu
cat package.json
find . -maxdepth 2 -type f \( -name 'package-lock.json' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' \) -print
sed -n '1,245p' tests/stations.test.ts
curl -fsSL https://raw.githubusercontent.com/dClimate/dclimate-client-js/dparquet/tests/stations.test.ts \
| rg -n -C 10 'withinRange|requireColumns|within.*columns|findNearestStation'Repository: dClimate/dclimate-client-js
Length of output: 12552
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- lockfile references ---'
rg -n -C 5 '`@dclimate/tabular`|dparquet' package-lock.json yarn.lock
printf '%s\n' '--- dparquet repository metadata ---'
curl -fsSL https://api.github.com/repos/dClimate/dparquet \
| jq '{full_name, default_branch, html_url}'
printf '%s\n' '--- dparquet tree entries ---'
branch=$(curl -fsSL https://api.github.com/repos/dClimate/dparquet \
| jq -r '.default_branch')
commit=$(curl -fsSL "https://api.github.com/repos/dClimate/dparquet/branches/$branch" \
| jq -r '.commit.sha')
curl -fsSL "https://api.github.com/repos/dClimate/dparquet/git/trees/$commit?recursive=1" \
| jq -r '.tree[] | select(.path | test("station|reader|package.json"; "i")) | .path' \
| head -120
printf '%s\n' '--- candidate guard locations from GitHub code search substitute ---'
for path in \
src/reader/stations.ts \
src/reader/station-dataset.ts \
src/stations.ts \
src/reader.ts \
tests/stations.test.ts \
tests/reader/stations.test.ts \
package.json
do
url="https://raw.githubusercontent.com/dClimate/dparquet/$branch/$path"
if body=$(curl -fsSL "$url" 2>/dev/null); then
printf '\n--- %s ---\n' "$path"
printf '%s\n' "$body" | rg -n -C 8 'withinRange|requireColumns|findNearestStation' || true
fi
doneRepository: dClimate/dclimate-client-js
Length of output: 2953
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 5 '`@dclimate/tabular`|dparquet' package-lock.json yarn.lock
curl -fsSL https://api.github.com/repos/dClimate/dparquet \
| jq '{full_name, default_branch, html_url}'Repository: dClimate/dclimate-client-js
Length of output: 2642
🌐 Web query:
"withinRange" "requireColumns" "@dclimate/tabular"
💡 Result:
Based on an analysis of the dClimate ecosystem and common TypeScript utility libraries, there is no evidence that withinRange or requireColumns are part of a public API or official library named @dclimate/tabular. The term requireColumns is a common utility function name in TypeScript/JavaScript development [1] and has been implemented as a lint rule in other open-source projects [2], but it is not associated with dClimate's documented SDKs or APIs [3][4][5][6]. The dClimate JavaScript/TypeScript SDKs (such as dclimate-client-js) primarily utilize classes like GeoTemporalDataset for handling geospatial and temporal data through methods like point, points, circle, rectangle, and timeRange [5]. If you are encountering these function names, they likely originate from: 1. Custom, project-specific utility code within your own codebase or a private repository [1]. 2. A different library entirely, potentially one unrelated to the dClimate climate data infrastructure [2]. For official dClimate integration, please refer to the dClimate documentation for the correct SDK methods and data querying patterns [3][4].
Citations:
- 1: https://github.com/adam-drag/logistics-ts/blob/main/packages/core/src/table/loader.ts
- 2: Add lint rule
requireColumnsk1LoW/tbls#95 - 3: https://docs.dclimate.net/docs/Develop/hacking-on-dclimate
- 4: https://docs.dclimate.net/docs/Develop/libraries/dclimate-sdk
- 5: https://github.com/dClimate/dclimate-client-js
- 6: https://pypi.org/project/dclimate-client-py/0.8.0/
🌐 Web query:
"dparquet" "findNearestStation"
💡 Result:
There is no standardized function or library feature named dparquet findNearestStation. The term dparquet does not appear to correspond to a widely recognized software library or function by that name. The phrase findNearestStation is a common, generic name for custom-implemented functions in various software development contexts, including: 1. Application Development: Developers often write custom JavaScript or other language-specific functions named findNearestStation to calculate the distance between a user's coordinate and a list of locations (such as transit stations or petrol stations) [1][2][3][4]. 2. Game Development: The term is explicitly documented in the Oolite JavaScript Reference as a built-in method for ship entities to return the nearest station [5]. 3. Misinterpretation: It is possible that dparquet is a misunderstanding of fastparquet, which is a legitimate and widely used Python library for reading and writing Parquet files [6][7][8]. However, fastparquet does not contain a findNearestStation function [8]. If you are encountering this in a specific codebase or technical problem, it is likely a custom utility function specific to that project [9][1].
Citations:
- 1: kwpark0047-iceu/c251229@9a88929
- 2: https://stackoverflow.com/questions/43353346/javascript-function-using-given-argument-object-sent-from-geolocation
- 3: https://grokipedia.com/page/Nearest_station_finder_Android
- 4: https://www.ambitionbox.com/interviews/question/given-the-locations-of-all-petrol-stations-in-a-city-and-the-cars-maximum-travel-distance-of-5-km-after-running-out-of-petrol-how-would-you-find-the-nearest-petrol-station-68TrIkBY
- 5: https://wiki.alioth.net/index.php/Oolite_JavaScript_Reference:_Ship
- 6: https://github.com/dask/fastparquet/
- 7: https://pypi.org/project/fastparquet/
- 8: https://fastparquet.readthedocs.io/en/stable/
- 9: kwpark0047-iceu/c251229@d08c36f
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository references and remotes ---'
git remote -v
rg -n -i 'dparquet|tabular|findNearestStation|withinRange|requireColumns' README.md docs .github src tests scripts \
-g '!node_modules'
printf '%s\n' '--- GitHub repository search ---'
curl -fsSLG https://api.github.com/search/repositories \
--data-urlencode 'q=dparquet org:dClimate' \
| jq -r '.items[] | [.full_name, .html_url, .default_branch] | `@tsv`'Repository: dClimate/dclimate-client-js
Length of output: 2223
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- origin branches containing dparquet ---'
git ls-remote --heads origin 'dparquet*' 'station*' 'feature*' | head -80
printf '%s\n' '--- dClimate repositories related to tabular/parquet ---'
curl -fsSL 'https://api.github.com/orgs/dClimate/repos?per_page=100' \
| jq -r '.[] | [.name, .default_branch, .html_url] | `@tsv`' \
| grep -Ei 'parquet|tabular|station|data' || true
printf '%s\n' '--- standalone option-shape verifier ---'
node - <<'JS'
function forwarded(request) {
return {
...(request.columns ? { requireColumns: request.columns } : {}),
...(request.maxKm === undefined ? {} : { maxKm: request.maxKm }),
...(request.within === undefined ? {} : { withinRange: request.within }),
};
}
const cases = [
{ name: 'within only', request: { within: { start: '2024-01-01', end: '2024-12-31' } } },
{ name: 'within with columns', request: {
columns: ['TMAX'],
within: { start: '2024-01-01', end: '2024-12-31' },
} },
{ name: 'empty columns with within', request: {
columns: [],
within: { start: '2024-01-01', end: '2024-12-31' },
} },
];
for (const { name, request } of cases) {
const options = forwarded(request);
console.log(name, JSON.stringify(options), {
hasWithinRange: Object.hasOwn(options, 'withinRange'),
hasRequireColumns: Object.hasOwn(options, 'requireColumns'),
requireColumnsLength: options.requireColumns?.length ?? null,
});
}
JSRepository: dClimate/dclimate-client-js
Length of output: 1157
🏁 Script executed:
#!/bin/bash
set -eu
git ls-remote --heads origin 'dparquet*' 'station*' 'feature*' | head -80
curl -fsSL 'https://api.github.com/orgs/dClimate/repos?per_page=100' \
| jq -r '.[] | [.name, .default_branch, .html_url] | `@tsv`' \
| grep -Ei 'parquet|tabular|station|data' || true
node - <<'JS'
function forwarded(request) {
return {
...(request.columns ? { requireColumns: request.columns } : {}),
...(request.maxKm === undefined ? {} : { maxKm: request.maxKm }),
...(request.within === undefined ? {} : { withinRange: request.within }),
};
}
for (const [name, request] of [
['within only', { within: { start: '2024-01-01', end: '2024-12-31' } }],
['within with columns', {
columns: ['TMAX'],
within: { start: '2024-01-01', end: '2024-12-31' },
}],
['empty columns with within', {
columns: [],
within: { start: '2024-01-01', end: '2024-12-31' },
}],
]) {
const options = forwarded(request);
console.log(name, JSON.stringify(options), {
hasWithinRange: Object.hasOwn(options, 'withinRange'),
hasRequireColumns: Object.hasOwn(options, 'requireColumns'),
requireColumnsLength: options.requireColumns?.length ?? null,
});
}
JSRepository: dClimate/dclimate-client-js
Length of output: 1015
🏁 Script executed:
#!/bin/bash
set -eu
commit=c9282d9902ece8e1a566e62e231f592127c879a
printf '%s\n' '--- dparquet branch tree ---'
curl -fsSL "https://api.github.com/repos/dClimate/dclimate-client-js/git/trees/$commit?recursive=1" \
| jq -r '.tree[] | [.type, .path, (.sha // "")] | `@tsv`' \
| grep -Ei '(^|/)(dparquet|tabular|station|reader)|package.json|gitmodules' \
| head -160
printf '%s\n' '--- branch package and submodule metadata ---'
for path in package.json .gitmodules; do
printf '\n--- %s ---\n' "$path"
curl -fsSL "https://raw.githubusercontent.com/dClimate/dclimate-client-js/dparquet/$path" 2>/dev/null || true
doneRepository: dClimate/dclimate-client-js
Length of output: 2626
🏁 Script executed:
#!/bin/bash
set -eu
commit=c9282d9902ece8e1a566e62e231f592127c879a
curl -fsSL "https://api.github.com/repos/dClimate/dclimate-client-js/git/trees/$commit?recursive=1" \
| jq -r '.tree[] | [.type, .path, (.sha // "")] | `@tsv`' \
| grep -Ei '(^|/)(dparquet|tabular|station|reader)|package.json|gitmodules' \
| head -160
for path in package.json .gitmodules; do
printf '\n--- %s ---\n' "$path"
curl -fsSL "https://raw.githubusercontent.com/dClimate/dclimate-client-js/dparquet/$path" 2>/dev/null || true
doneRepository: dClimate/dclimate-client-js
Length of output: 2551
Reject within without a non-empty columns array.
When within is set, nearest forwards withinRange without requireColumns, or with an empty array. Validate this before load and throw InvalidSelectionError so no gateway request occurs.
🤖 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 `@src/stations/stations-client.ts` at line 62, Update nearest and its selection
validation so any request with within requires a non-empty columns array before
load is called. Throw InvalidSelectionError for missing or empty columns, and
ensure the invalid path does not forward withinRange or issue a gateway request.
| "@dclimate/jaxray": "^0.7.0", | ||
| "@opentelemetry/api": "^1.9.1" | ||
| "@opentelemetry/api": "^1.9.1", | ||
| "@dclimate/tabular": "file:../dparquet", |
There was a problem hiding this comment.
HIGH
file:../dparquet makes installs depend on an unpublished sibling directory. A fresh clone and the published npm package will not contain ../dparquet, so npm install fails. Depend on a published version, workspace package included in the repository, or bundled tarball.
| gatewayUrl: request.gatewayUrl ?? this.options.gatewayUrl, | ||
| ...(this.options.fetch ? { fetch: this.options.fetch } : {}), | ||
| }); | ||
| return StationDataset.open(source, root); |
There was a problem hiding this comment.
MEDIUM
StationDataset.open() can reject with DatasetReaderError when a valid CID points to an invalid station manifest, but this call is outside nearest()'s translation block. Those failures leak the dependency's error instead of a DClimateClientError; await and translate reader errors here.
| // Either bound alone is meaningful, so the missing side widens to the | ||
| // dataset's own extent rather than forcing the caller to pass both. | ||
| // Widening costs a full index walk, so only the open-ended side pays for it. | ||
| const covered = await dataset.listStations(); |
There was a problem hiding this comment.
MEDIUM
Every ranged query walks the complete station index even when both --from and --to are supplied. For GHCND this adds roughly 136k unnecessary reads, and spreading that many timestamps into Math.min/Math.max can exceed V8's argument limit. Only enumerate coverage for a missing bound and compute extrema iteratively.
| "prepare": "npm run build", | ||
| "prepublishOnly": "npm run build" | ||
| "prepublishOnly": "npm run build", | ||
| "inspect:stations": "tsx scripts/inspect-stations.ts" |
There was a problem hiding this comment.
MEDIUM
The new npm script invokes tsx, but tsx is not installed as a dependency or dev dependency, so npm run inspect:stations fails with tsx: not found after a normal install. Add it to devDependencies.
# Conflicts: # README.md # src/client.ts
| "dependencies": { | ||
| "@dclimate/jaxray": "^0.7.0", | ||
| "@opentelemetry/api": "^1.9.1" | ||
| "@dclimate/tabular": "file:../dparquet", |
There was a problem hiding this comment.
HIGH
file:../dparquet only resolves when an untracked sibling checkout exists. Clean clones and CI installs will fail, and a published package would leave consumers with an unresolvable local dependency. Use a published @dclimate/tabular version or include it in this repository's workspace.
| // like any other reader failure: a caller catching `DClimateClientError` | ||
| // should not have to also know `@dclimate/tabular`'s error hierarchy. | ||
| try { | ||
| return await StationDataset.open(source, root); |
There was a problem hiding this comment.
MEDIUM
load() returns the raw StationDataset, so only failures during open() are translated. Errors from the advertised chained methods such as select() or rows() still escape as tabular errors and cannot be caught as DClimateClientError. Return a wrapper that translates query failures or expose the original error contract explicitly.
| const stations = await client.stations.load({ cid: "bafyr4i..." }); | ||
|
|
||
| // Every station, with position and coverage window. | ||
| for (const s of stations.stations) { |
There was a problem hiding this comment.
LOW
StationDataset does not expose a synchronous stations collection; metadata is obtained asynchronously through listStations(). This example therefore fails when copied. Iterate over await stations.listStations() instead.
Replaces the file:../dparquet path dependency, which broke when that repo was renamed to tabular-js. Both lockfiles regenerated.
| "dependencies": { | ||
| "@dclimate/jaxray": "^0.7.0", | ||
| "@opentelemetry/api": "^1.9.1" | ||
| "@dclimate/tabular": "^0.1.0", |
There was a problem hiding this comment.
HIGH
@dclimate/tabular@0.1.0 requires Node >=22, while both repository workflows install under Node 20. Yarn rejects the dependency before tests run, and both checks currently fail with the engine mismatch. Use a Node-20-compatible release or update the declared/runtime support and CI to Node 22.
| // like any other reader failure: a caller catching `DClimateClientError` | ||
| // should not have to also know `@dclimate/tabular`'s error hierarchy. | ||
| try { | ||
| return await StationDataset.open(source, root); |
There was a problem hiding this comment.
MEDIUM
load() returns the raw tabular StationDataset, so errors from subsequent select(), timeRange(), nearest(), plan(), or rows() calls bypass translateStationError. Consequently, the advertised instanceof DClimateClientError handling fails for normal station queries. Return a client-owned wrapper/proxy that translates these method failures.
| const stations = await client.stations.load({ cid: "bafyr4i..." }); | ||
|
|
||
| // Every station, with position and coverage window. | ||
| for (const s of stations.stations) { |
There was a problem hiding this comment.
LOW
StationDataset has no stations property; station enumeration is exposed as the async listStations() method. This example throws because undefined is not iterable. Iterate over await stations.listStations() instead.
| ```typescript | ||
| const hotDays = await stations | ||
| .nearest(29.98, -95.36) | ||
| .timeRange({ start: "2025-01-01", end: "2025-12-31" }) |
There was a problem hiding this comment.
LOW
nearest() returns Promise<StationDataset>, so this attempts to call timeRange() on a Promise. Await the nearest selection first, for example (await stations.nearest(...)).timeRange(...).
| let start: Date | string = args.from ?? ""; | ||
| let end: Date | string = args.to ?? ""; | ||
| if (!args.from || !args.to) { | ||
| const covered = await dataset.listStations(); |
There was a problem hiding this comment.
MEDIUM
A one-sided --from or --to query calls listStations() on the entire original dataset, even after --station/--near narrowed the selection. For GHCND this incurs the documented ~136k gateway reads before querying any rows. Use an unbounded sentinel/manifest extent, or at least derive coverage from the selected stations.
| ): Promise<StationDataset> { | ||
| let selected = dataset; | ||
|
|
||
| if (args.near) { |
There was a problem hiding this comment.
LOW
When both --near and --station are supplied, this branch wins and the else if silently ignores every requested station. Reject these mutually exclusive options during argument validation so the command cannot return a plausible result for a different selection.
| } | ||
| // Anything else is not ours to reinterpret: a TypeError from a bug, or a | ||
| // network failure from the gateway, should surface as itself. | ||
| throw cause; |
There was a problem hiding this comment.
MEDIUM
Valid CIDs containing non-station data fail during StationDataset.open() with tabular CodecError or WireError. Neither matches the branches above, so this fallback leaks a foreign error instead of the promised DClimateClientError. Translate structural decoding errors to DatasetCorruptError while preserving transport failures.
|
|
||
| const started = Date.now(); | ||
| const element = args.elements.length === 1 ? args.elements[0] : undefined; | ||
| const records = await selected.toRecords(element); |
There was a problem hiding this comment.
MEDIUM
--limit only slices after toRecords() materializes every matching record. The default <cid> invocation therefore attempts to download and retain the entire dataset despite printing ten rows, which can exhaust memory on GHCND-scale data. Push a limit/stream into retrieval or refuse unbounded execution.
| case "--to": args.to = value(); break; | ||
| case "--limit": args.limit = Number(value()); break; | ||
| case "--plan": args.plan = true; break; | ||
| case "--list": args.list = true; break; |
There was a problem hiding this comment.
MEDIUM
Number("") is 0, so inputs such as --near ,-73.97 or --near 40.78, pass this validation and silently query a different location. Require both components to be non-empty and validate latitude/longitude bounds before assignment.
| // Stored in NOAA's own scaling rather than converted, so the archive's exact | ||
| // integers survive. Saying so beats letting a reader assume whole °C. | ||
| if (element) { | ||
| console.log(`\nValues are integers in tenths (TMAX 317 = 31.7 °C).`); |
There was a problem hiding this comment.
LOW
This unit message is printed for every single-element query, but station elements do not share one scale and tabular also supports non-integer columns. For example, SNOW is not temperature in tenths. Derive units from metadata or restrict this message to known temperature elements.
| // tabular's errors too, and a caller has no reason to expect the boundary to | ||
| // stop at `open`. | ||
| try { | ||
| return wrapStationDataset(await StationDataset.open(source, root)); |
There was a problem hiding this comment.
MEDIUM
StationDataset.open() defaults to the GHCND uppercase column mapping, and this loader exposes no way to supply columnKey. Mixed-case profiles supported by tabular, such as NDBC's SwH and SwD, therefore cannot be queried using their declared names and may suffer case collisions. Add a profile/columnKey option and pass it to open().
| } | ||
| // Other reader failures are malformed requests too -- an unknown column, a | ||
| // predicate against a column that is not comparable. | ||
| if (cause instanceof DatasetReaderError || cause instanceof PredicateError) { |
There was a problem hiding this comment.
MEDIUM
DatasetReaderError is not limited to invalid requests: tabular also throws it for corrupt fragment/schema conditions such as unknown field IDs, invalid decoded cell types, and undeclared Parquet ranges. Mapping the entire class to InvalidSelectionError misclassifies damaged data. Restrict this mapping to request errors or have those storage failures promoted to DatasetIntegrityError upstream.
| const parts = value().split(","); | ||
| const lat = Number(parts[0]); | ||
| const lon = Number(parts[1]); | ||
| if (parts.length !== 2 || !Number.isFinite(lat) || !Number.isFinite(lon)) { |
There was a problem hiding this comment.
MEDIUM
This accepts malformed coordinates because Number("") is 0, so --near ,-73.97 silently queries the equator. It also accepts latitude/longitude outside [-90, 90] and [-180, 180]; nearest-station search does not validate those bounds. Require non-empty components and valid geographic ranges.
| if (!args.from) start = new Date(earliest); | ||
| if (!args.to) end = new Date(latest); | ||
| } | ||
| selected = selected.timeRange({ start, end }); |
There was a problem hiding this comment.
MEDIUM
A one-sided range outside the selected coverage becomes inverted. For example, --from 2030-01-01 on a station ending in 2025 produces {start: 2030, end: 2025}; tabular's timeRange() swaps inverted bounds, returning 2025–2030 rows instead of no data. Detect an out-of-coverage bound before calling timeRange().
| console.log(`\nQuery (wire units): ${JSON.stringify(selected.toQuery())}`); | ||
|
|
||
| const plan = await selected.plan(); | ||
| const bytes = plan.fragments.reduce((sum, f) => sum + f.byteLength, 0); |
There was a problem hiding this comment.
MEDIUM
byteLength is the full Parquet fragment size, while the reader fetches only the planned ranges for selected and predicate columns. This can trip --max-bytes and reject a narrow query whose actual transfer is far below the ceiling. Estimate ranged reads from f.ranges and account separately for gateways that ignore ranges.
| // Stored in NOAA's own scaling rather than converted, so the archive's exact | ||
| // integers survive. Saying so beats letting a reader assume whole °C. | ||
| if (element) { | ||
| console.log(`\nValues are integers in tenths (TMAX 317 = 31.7 °C).`); |
There was a problem hiding this comment.
LOW
This message is printed for every single-element dataset, but tabular supports float and string station columns (for example NDBC data), not only NOAA scaled integers. It reports false units for those datasets. Derive units/type from metadata or limit the message to known GHCND elements.
| // tabular's errors too, and a caller has no reason to expect the boundary to | ||
| // stop at `open`. | ||
| try { | ||
| return wrapEntityDataset(await EntityDataset.open(source, root)); |
There was a problem hiding this comment.
MEDIUM
EntityDataset.open defaults columnKey to the schema field name, but this loader neither forwards nor exposes a mapping. The GHCND entity profile uses lowercase schema fields while exposing NOAA names such as TMAX through columnKey, so the documented TMAX queries fail as unknown elements. Add a load option/profile mapping and pass it to open.
| ? (await dataset.findNearestEntity(args.near[0], args.near[1])).entityId | ||
| : (await dataset.listEntities())[0]?.entityId); | ||
| if (reference !== undefined) { | ||
| referenceColumns = await dataset.columnsFor(reference); |
There was a problem hiding this comment.
MEDIUM
Element names are validated against one reference entity's columnsFor result, which contains only columns that entity has reported. If the unfiltered nearest entity or first --entity lacks TMAX, the command rejects it before it can find another selected entity that reports it. Resolve names against the dataset schema/global vocabulary rather than one entity.
| } | ||
| // Other reader failures are malformed requests too -- an unknown column, a | ||
| // predicate against a column that is not comparable. | ||
| if (cause instanceof DatasetReaderError || cause instanceof PredicateError) { |
There was a problem hiding this comment.
MEDIUM
DatasetReaderError also represents stored-data failures in tabular 0.8, including unknown field IDs in fragment metadata, undeclared Parquet ranges, and values inconsistent with the schema. Mapping the entire class to InvalidSelectionError misreports corrupt datasets as caller mistakes. Ensure storage violations use/map to DatasetCorruptError while retaining invalid-query handling separately.
| DEFAULT_STAC_SERVER_URL, | ||
| } from "./stac/stac-server.js"; | ||
| import { SirenClient } from "./siren/siren-client.js"; | ||
| import { EntitiesClient } from "./entities/entities-client.js"; |
There was a problem hiding this comment.
MEDIUM
This static import defeats the getter's intended lazy loading: importing the main client also evaluates entities-client, which statically pulls in the tabular reader, hyparquet, and its compressors. Existing browser and Node consumers pay the added startup/bundle cost even when they never access client.entities. Move the heavy reader imports behind load() or isolate entities in a lazy subpath.
| case "--max-bytes": args.maxBytes = Number(value()); break; | ||
| case "--near": { | ||
| const parts = value().split(","); | ||
| const lat = Number(parts[0]); |
There was a problem hiding this comment.
LOW
Number("") is 0, so inputs such as --near ,-73.97 or --near 40.78, are silently accepted as valid coordinates and query the wrong location. Reject empty trimmed components before numeric conversion.
| if (!args.from) start = new Date(earliest); | ||
| if (!args.to) end = new Date(latest); | ||
| } | ||
| selected = selected.timeRange({ start, end }); |
There was a problem hiding this comment.
MEDIUM
EntityDataset.timeRange() swaps inverted endpoints. With only --from after the selected coverage ends (or --to before it starts), the inferred missing bound creates an inverted range, causing the command to query the opposite interval and potentially return rows outside the request. Reject out-of-coverage one-sided bounds before calling timeRange().
| console.log(`\nQuery (wire units): ${JSON.stringify(selected.toQuery())}`); | ||
|
|
||
| const plan = await selected.plan(); | ||
| const bytes = plan.fragments.reduce((sum, f) => sum + f.byteLength, 0); |
There was a problem hiding this comment.
MEDIUM
byteLength is the full Parquet fragment size, while the reader fetches only the planned ranges. Summing full fragment sizes makes --max-bytes reject narrow queries whose actual transfer is far below the ceiling. Calculate the ranged-read estimate from f.ranges, handling range-ignoring gateways separately.
| } | ||
| const lat = Number(parts[0]); | ||
| const lon = Number(parts[1]); | ||
| if (!Number.isFinite(lat) || !Number.isFinite(lon)) { |
There was a problem hiding this comment.
LOW
This validation accepts finite but invalid coordinates such as latitude 100 or longitude 250; the nearest-entity implementation does not reject these bounds and can return a plausible station for a nonexistent location. Require latitude within [-90, 90] and longitude within [-180, 180].
| // No translation boundary of its own: `load` returns the dataset already | ||
| // wrapped, so `findNearestEntity`'s failures come back translated. | ||
| const dataset = await this.load(request); | ||
| return dataset.findNearestEntity(request.latitude, request.longitude, { |
There was a problem hiding this comment.
MEDIUM
findNearestEntity accepts finite but out-of-range coordinates, so requests such as latitude 100 can return a plausible station for a nonexistent location. Validate latitude within [-90, 90] and longitude within [-180, 180] before delegating.
| // queries whose actual transfer was far below the ceiling. Assumes a gateway | ||
| // that honours Range; one that ignores it sends whole fragments, which the | ||
| // reader itself then caps per response. | ||
| const bytes = plan.fragments.reduce( |
There was a problem hiding this comment.
MEDIUM
--max-bytes sums only planned range slices, but GatewayRangeSource accepts whole-fragment 200 responses when a gateway ignores Range. Many small planned ranges can therefore pass the ceiling while transferring gigabytes. Enforce the cap at the transport layer or require 206 responses when it is enabled.
| // queries whose actual transfer was far below the ceiling. Assumes a gateway | ||
| // that honours Range; one that ignores it sends whole fragments, which the | ||
| // reader itself then caps per response. | ||
| const bytes = plan.fragments.reduce( |
There was a problem hiding this comment.
MEDIUM
--max-bytes sums only the planned range slices, but GatewayRangeSource accepts a whole-fragment 200 response when a gateway ignores Range. Across many fragments, this can approve a query estimated below 512 MiB while actually transferring gigabytes. Enforce the cumulative limit in the transport, require 206 responses when the cap is enabled, or conservatively account for full-body fallbacks.
| if (!args.from) start = new Date(earliest); | ||
| if (!args.to) end = new Date(latest); | ||
| } | ||
| selected = selected.timeRange({ start, end }); |
There was a problem hiding this comment.
MEDIUM
EntityDataset.timeRange() swaps inverted endpoints. Because both supplied bounds are not validated here, --from 2025-12-31 --to 2025-01-01 silently queries January through December instead of rejecting the typo. Compare parsed endpoints before delegating.
| // queries whose actual transfer was far below the ceiling. Assumes a gateway | ||
| // that honours Range; one that ignores it sends whole fragments, which the | ||
| // reader itself then caps per response. | ||
| const bytes = plan.fragments.reduce( |
There was a problem hiding this comment.
MEDIUM
--max-bytes is not a reliable ceiling because this estimate counts only requested ranges. GatewayRangeSource accepts full-fragment 200 responses when a gateway ignores Range, so many small planned ranges can pass the check while transferring gigabytes cumulatively. Enforce the limit in the transport or require 206 responses while the cap is enabled.
1 participant
Summary by CodeRabbit
New Features
client.stations, including CID loading and gateway configuration.Documentation