fix: initial dparquet (not done) - #10
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 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 | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![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.
| - **`where(...)` has no gridded counterpart.** Row-level predicates are pushed | ||
| down to fragment statistics, so most fragments are skipped without being read: | ||
|
|
||
| ```typescript | ||
| 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 | ||
| .rows(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '\bwhere\s*\(|fragment|statistics|predicate|skip' src testsRepository: dClimate/dclimate-client-js
Length of output: 24478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files likely to define where/rows/fetch paths =="
git ls-files | rg '(^|/)(README\.md|.*\.ts)$' | rg 'src |package.json|tests' | sed -n '1,200p'
echo
echo "== find all identifiers that export/assign 'where' or related query builder methods =="
rg -n "where:|method:.*where|\\.where\\(|function where|const where|rows\\(|nearest\\(|timeRange\\(" src tests --glob '*.ts' --glob '!src/shapes/circle.ts' --glob '!src/actions/concatenate-variants.ts' | sed -n '1,240p'Repository: dClimate/dclimate-client-js
Length of output: 4384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/geotemporal-dataset.ts outline =="
ast-grep outline src/geotemporal-dataset.ts || true
echo
echo "== src/geotemporal-dataset.ts relevant section =="
sed -n '1,340p' src/geotemporal-dataset.ts | cat -n
echo
echo "== occurrences of API query predicates in src and tests =="
rg -n "where|op:|value:|element|statistic|min|max|stats|bounds|bbox|spatialExtent|fragment|skip" src tests --glob '*.ts' | sed -n '1,320p'Repository: dClimate/dclimate-client-js
Length of output: 28615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== README.md around documented API usage =="
sed -n '90,130p' README.md | cat -n
echo
echo "== all source/test references to rows, station, nearest, and predicate shapes =="
rg -n "\.rows\(|stations|nearest\(|where\(|{ element:|element.*op:|op: .*value:|\.where\(" . --glob '*.ts' --glob '*.md' --glob '!package-lock.json' --glob '!node_modules/**' | sed -n '1,320p'
echo
echo "== git tracked ts files under src with top-level exports/classes likely query API =="
git ls-files src/*.ts src/**/*.ts | sed -n '1,200p'Repository: dClimate/dclimate-client-js
Length of output: 2284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all source/test references to stations, nearest, rows, and where =="
python3 - <<'PY'
import subprocess
patterns = [r"\.rows\(", r"stations", r"\.nearest\(", r"\.where\(", r"\brows\(", r"\bnearest\("]
for pattern in patterns:
print(f"\n-- pattern: {pattern} --")
try:
subprocess.run([
"rg", "-n", "-C", "4", pattern, ".",
"--glob", "*.ts", "--glob", "*.md",
"--glob", "!package-lock.json",
"--glob", "!node_modules/**"
], check=False)
except subprocess.CalledProcessError as e:
if e.returncode != 1: raise
PYRepository: dClimate/dclimate-client-js
Length of output: 23875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files under src/stations =="
git ls-files src/stations | sort
echo
echo "== inspect relevant station files excluding data files =="
for f in $(git ls-files 'scripts/inspect-stations.ts' 'src/stations/**/*.ts' | grep -v '\.parquet$' | grep -v '\.json$'); do
echo "--- $f ($(wc -l < "$f") lines) ---"
ast-grep outline "$f" || true
done
echo
echo "== search for where/predicate/statistics implementation in src scripts =="
rg -n -C 6 "where|predicate|element.*op:|op:.*element|fragment.*statistic|statistics|min|max|bounds|selectFragments|planFragments|toQuery|nearest|rows|rowCount|min|max|min_value|max_value|byteLength" scripts src --glob '*.ts' --glob '!src/shapes/circle.ts' | sed -n '1,500p'Repository: dClimate/dclimate-client-js
Length of output: 31577
Document where(...) as an external optimization, not a client-side guarantee.
The SDK exports client.stations.load(), but the documented .where(...).rows() API and the described fragment-stats pruning are not present in this code. Qualify the guarantee by ownership, or make the documented client paths implement and test that behavior.
🤖 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 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.
| 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.
| if (args.near) { | ||
| const [lat, lon] = args.near; | ||
| selected = selected.nearest(lat, lon); | ||
| console.log(`\nNearest station to ${lat}, ${lon}: ${selected.toQuery().stations?.[0]}`); | ||
| } else if (args.stations.length > 0) { | ||
| selected = selected.select(...args.stations); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject conflicting station selectors.
When callers pass both --near and --station, --near silently ignores every station ID. Reject this combination, or document and implement an explicit combined-selection behavior.
Proposed fix
function applySelection(dataset: StationDataset, args: Args): StationDataset {
+ if (args.near && args.stations.length > 0) {
+ throw new Error("--near cannot be used with --station");
+ }
+
let selected = dataset;🤖 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 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.
| "@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).
1 participant
Summary by CodeRabbit
New Features
client.stations, including CID loading and gateway configuration.Documentation