fix(cubesql): Preserve lastRefreshTime with post-processing and SQL pushdown

[MazterQyou]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Description of Changes Made

This PR makes the SQL API return lastRefreshTime for queries whose plan has post-processing nodes over the cube scan (calculated projections over MEASURE(), sorts, filters) and for SQL pushdown queries, both of which silently dropped it. Related test is included.

[claude]

Claude finished @MazterQyou's task in 4m 54s —— View job

---

Review complete — approach is sound; 1 medium (external aggregation direction), 3 low/nits, posted as 4 inline comments

Full review — traced the plumbing end to end, findings and verification notes

• Gather context (read diff + surrounding code)
• Trace the freshness plumbing end to end (gateway → transport → scan → span → JSONL header)
• Review Rust changes (scan.rs, service.rs, node_export.rs)
• Review TS change (gateway.ts)
• Review tests
• Post inline comments + summary

Verification

I traced both halves of the fix rather than taking the description at face value:

SQL pushdown path. scan.rs → transport.load(sql_query: Some(..)) → JS sqlApiLoad → gateway's request.sqlQuery branch. Because that branch resolves a plain object (not a ResultWrapper), wrapNativeFunctionWithStream takes the JSON.stringify path (js/index.ts:335), so it lands in ValueFromJs::String → TransportLoadResponseColumnar → convert_transport_response → build_response_schema (transport.rs:478-527, scan.rs:1307). V1LoadResult already has lastRefreshTime/external (v1_load_result.rs:31-38) — they were simply never populated on the pushdown side. Confirms the gateway.ts two-liner is the correct and complete fix for that path.

Post-processing path. Confirmed the mechanism: CubeScanExecutionPlan::execute returns a router whose schema is the batch schema (scan.rs:588-596), so the metadata only reached stream.schema() when the scan was the plan root. A ProjectionExec/SortExec on top rebuilds the schema and drops it. Routing through the span is the right fix, and it also improves the pg-wire path (shim.rs:390, 1878) for free.

Format. Span round-trips through DateTime::parse_from_rfc3339 → to_rfc3339_opts(Millis, true). Since the value originates from JS toISOString(), output is byte-identical to the old schema-metadata path — no observable change for existing consumers. Unparseable values fall back to the schema metadata via or_else, which is correct.

Findings

#	Severity	Where	Issue
1	Medium	service.rs:128-140	external aggregates "any wins" while last_refresh_time right above aggregates "oldest wins". A join where one scan hits CubeStore and one hits the source DB reports external: true on a partially-live result. Elsewhere the flag is computed with .all(...) (base_query.rs:84, top_level_planner.rs:56). Needs a tri-state to be conservative — or an explicit note that "any" is deliberate.
2	Low	node_export.rs:377-386	The span read happens before the first stream.next(), so it works only because execute is async in this DataFusion fork and parents await children eagerly. Undocumented load-bearing assumption for a header that must be written first. Also, the comment's "every cube scan that ran" isn't true in stream mode (scan.rs:554 never calls load_data) — not a regression, but the comment overstates coverage.
3	Low	gateway.ts:2228-2233	disableExternalPreAggregations is set for pushdown queries, so response.external should always be falsy here. Harmless to mirror, but worth a note — the two new tests read as if both true and false are live cases.
4	Nit	sql-api-load.test.ts:64-78	No case for a response without lastRefreshTime — the common path, and the one that exercises the ?. and the undefined-is-dropped serialization contract. Plus a stray import * as console.

Also minor, not worth an inline: span_id_for_schema (node_export.rs:306) is a second Arc clone alongside span_id_clone; one clone reused would do.

What's good

• Test design is deliberate: the plain measure projection case in the parameterized native test is a real control that passed before the fix, so the pair actually isolates the regression rather than just asserting the happy path.
• The SpanId doc comments explain the aggregation semantics, not just the mechanics — that's what made finding ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 reviewable at all.
• Fallback ordering (span.or_else(schema)) is the right precedence and keeps plans without a span working.

Not verified

I could not execute either test suite — node_modules is not installed in this checkout and a Rust build wasn't attempted. All findings are from reading code; the ordering analysis in #2 is from tracing execute() call sites, not from running the plans.

· branch `cubesql/preserve-lastrefreshtime`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.43%. Comparing base (4283244) to head (34e7db2).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11394      +/-   ##
==========================================
+ Coverage   79.41%   79.43%   +0.02%     
==========================================
  Files         480      480              
  Lines       98646    98679      +33     
  Branches     3636     3636              
==========================================
+ Hits        78338    78390      +52     
+ Misses      19790    19768      -22     
- Partials      518      521       +3     

Flag	Coverage Δ
cube-backend	59.10% <ø> (+0.10%)	⬆️
cubesql	83.94% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @MazterQyou's task in 1m 59s —— View job

---

Re-review of the update — all 4 prior findings addressed; 1 new low (external fallback uses || instead of span-precedence)

Full re-review — what changed, verification, and the one new finding

• Diff the current head against my previous review
• Check which prior threads are now addressed
• Re-review the changed code
• Resolve stale threads / post new finding

Prior findings — all resolved

#	Prior finding	Status
1	external aggregated "any wins" vs last_refresh_time's "oldest wins"	Fixed. SpanId::external is now RWLockAsync<Option<bool>> with *write = Some(write.unwrap_or(true) && external) — the conservative tri-state. span_id_external_only_when_every_load_is_external covers the sticky-false case (true → false → true stays false), which is the part that's easy to get wrong. Doc comment now states the semantics and cross-references usages.iter().all(..).
2	Undocumented eager-child-execution assumption + comment overstated coverage	Fixed. The comment in node_export.rs:381-392 now spells out both the async execute dependency ("a node that deferred child execution to first poll would silently empty this header again, and the tests would not catch it") and the stream-mode gap.
3	external unreachable-as-true on the pushdown branch	Fixed. gateway.ts:2233-2238 explains the disableExternalPreAggregations chain and that the mirror is for shape parity. The test mock carries the same note.
4	Missing negative test case + stray import * as console	Fixed. omits lastRefreshTime when absent added, with a comment tying it to JSON.stringify dropping undefined → Option<String> on the Rust side. Import removed.

Resolved all four threads.

New finding

Low — node_export.rs:412-420: the external schema fallback uses ||, which can undo the aggregation that was just made conservative.

Two blocks up, last_refresh_time uses or_else: span wins outright, schema is consulted only when the span is silent. external ORs them instead, so a span that deliberately folded to false can be overridden by leftover schema metadata.

Reachable case: a UNION of two cube scans, first from a CubeStore pre-aggregation, second live. UnionExec delegates to child streams rather than rebuilding a batch schema, so stream.schema() is the first child's batch schema and still carries external=true. The span correctly says false; the || re-reports external: true alongside lastRefreshTime = min(A, B) — precisely the disagreement the new set_external doc comment argues against.

Fix needs the tri-state to leak out (external() -> Option<bool>, .unwrap_or_else(schema), with shim.rs callers taking .unwrap_or(false)). Details in the inline comment. Narrow in practice, but the two adjacent blocks reading differently is the kind of thing that gets copied.

Fix this →

Also checked, no issue

• set_external read-modify-write happens under a single write guard, so concurrent loads can't interleave the fold.
• response.lastRefreshTime?.toISOString() — QueryOrchestrator.ts:262/287 produces a Date, and this matches the existing prepareResultTransformData call at gateway.ts:1935.
• set_last_refresh_time still only fires on a parseable RFC3339 value while set_external fires on every load; asymmetric but correct — a missing timestamp shouldn't make the span look fresher.
• span_id_for_schema is still a second Arc clone alongside span_id_clone; cosmetic, not worth a thread.

Not verified

Could not execute either suite — no node_modules in this checkout and no Rust build attempted. The UnionExec schema-delegation claim is from reading the operator's stream handling, not from running a union plan.

· branch `cubesql/preserve-lastrefreshtime`

[claude]

The schema fallback for external uses ||, which can undo the conservative aggregation that SpanId::set_external was just changed to do.

last_refresh_time above uses or_else — span wins outright, schema is consulted only when the span is silent. external instead ORs the two, so a span that deliberately reported false can still be overridden to true by leftover schema metadata.

Concrete case: a UNION of two cube scans, first served from a CubeStore pre-aggregation, second live. UnionExec delegates to its child streams rather than rebuilding a batch schema, so stream.schema() here is the first child's batch schema and carries external=true. The span correctly folded to false (not every load was external), but the || re-reports external: true for a partially-live result — exactly the disagreement with lastRefreshTime = min(A, B) that the new doc comment on set_external argues against.

Matching the precedence used two blocks up needs the tri-state to leak out, e.g.:

pub async fn external(&self) -> Option<bool> { *self.external.read().await }

let external = span_external.unwrap_or_else(|| {
    stream.schema().metadata().get("external").map(|v| v == "true").unwrap_or(false)
});
if external { /* insert */ }

(shim.rs callers would need .unwrap_or(false).) Low severity given how narrow the reachable case is, but the two adjacent blocks reading differently is the kind of thing that gets copied.
true