Type the instant cell: one unit from storage to every protocol edge - #336
Merged
Merged
Conversation
…eTime Value::DateTime and Value::NaiveDateTime now serialize to a dedicated fixext8 ext type (UTC vs naive) carrying epoch microseconds, instead of a formatted string. This gives byte-decodable comparison and lets the JSON transcoder render the value as ISO 8601 without a full msgpack-to-Value decode. - nodedb-types::json_msgpack::instant_ext defines the encode/decode and InstantKind, re-exported as write_instant/read_instant/InstantKind from nodedb-types; Value gains as_instant() to extract either variant. - The transcoder recognizes the instant ext and writes an ISO 8601 string; any other ext type still renders as null. - nodedb-query's msgpack_scan writer/compare gain matching write_instant/write_kv_instant helpers and an instant type rank so raw-byte field comparison orders instants correctly (by kind, then signed micros) instead of falling into the generic ext bucket. - Split the single-file json_msgpack/reader.rs and msgpack_scan/reader.rs into per-concern submodules (cursor/json/native and tags/scalar/skip/value respectively); no behavior change beyond the module boundary.
Add coverage that a TIMESTAMP time key or column denotes the same stored instant regardless of the route that reads it: direct SELECT, INSERT ... RETURNING, an INNER JOIN (local, shuffle, and cross-node), time_bucket, date_part, and GROUP BY. Cases span a document_strict TIMESTAMP column (strict_typed_column_rendering.rs, new), a timeseries TIME_KEY joined against another timeseries collection or a document_strict TIMESTAMP column, and single-node vs 3-node cluster cross-node joins comparing TIME_KEY columns in their ON predicate.
Datetime scalar functions (datetime, extract, date_trunc, date_add, date_sub, date_diff, time_bucket) previously required string arguments and always parsed/returned via string ISO 8601, discarding whether the value was a UTC or naive instant. Add instant_arg to accept Value::DateTime, Value::NaiveDateTime, or a parseable string, and have each function return the same instant kind it received. time_bucket now buckets typed instants on epoch microseconds (floored toward negative infinity) while still supporting integer millisecond timestamps bucketed on epoch milliseconds.
…trings RowsPayload cells (RETURNING, RLS row values, trigger batches, bulk DML, merge, timeseries scan/sort/merge) move from pre-rendered Option<String> TEXT cells to NativeCell, a Value written in plain (untagged) msgpack. NativeCell shares its plain-msgpack reader/writer with value_to_msgpack/value_from_msgpack via a native decoder now generic over zerompk::Read, so both entry points produce identical bytes and agree on decode. RETURNING now retypes only text cells found under a numeric, bool or timestamp column instead of every cell, since typed cells already carry their number/bool/instant form. nodedb/src/util/rmpv_value.rs centralizes the rmpv::Value <-> nodedb_types::Value conversion (including instant ext round-tripping) used by timeseries rows, trigger batches and RETURNING projections, replacing the ad-hoc converter that lived in the trigger batch collector.
Replace the unit-less TsColumnType::Timestamp with Timestamp(TimeKind), where TimeKind::Instant carries InstantKind::Naive/Utc and TimeKind::Millis marks a declared integer time column. The columnar bridge, segment codec, schema evolution, sparse index, grouped scan, ILP ingest, and detection paths now key off this kind instead of assuming every timestamp column is the same representation, so a UTC instant maps to Timestamptz and a millis column keeps its Int64 storage instead of being coerced to a bare timestamp. Split columnar_memtable/memtable.rs and grouped_scan/strategies.rs into per-concern submodule directories.
Time columns now render as their declared kind (typed instant or raw milliseconds) at the point a scan emits them, using the column's own TimeKind rather than the ts_instant_columns() name list. Columnar memtable and partition row emission, aggregate/sort/ingest paths, and msgpack filter/group-key/value-ops helpers thread this typed rendering through. This removes the join-side instant rescale pass entirely: a join no longer needs to know which locally-scanned columns are instants and rewrite them after emission, since every row already carries correctly typed time cells by the time a join reads it. instant_scale.rs and its threading through join params/dispatch are deleted.
The segment reader infers a column's physical kind from its codec, so a declared TIMESTAMP/TIMESTAMPTZ column decodes as the same Int64 kind as any other eight-byte integer column, losing the instant/millis distinction the row emitter needs. ColumnType::instant_kind() and time_cell() give every reader (segment decode, memtable access, PK encoding) one place to type a stored time cell from the schema's declared type: Timestamp yields a naive instant, Timestamptz a UTC instant, and every other type (SystemTimestamp, Duration, the bitemporal audit columns) keeps the raw integer. decoded_col_to_value and ColumnData::get_value now take the column's declared ColumnType and merge the Int64/Timestamp decode arms, since both read the same physical storage. row_to_projected_json is renamed to row_to_projected_value and returns a nodedb_types::Value directly instead of routing through serde_json, since every caller already works with typed row values and now reuses the shared decoder for the DML predicate row read as well as the base scan. DecodedColumn drops #[non_exhaustive] now that every reader matches it exhaustively. columnar_read/scan.rs is split into a scan/ directory (execute, order, params, block_skip) to stay within the file size limit as its callers change.
Row-level-security FOR READ policies now govern SELECT results on columnar and spatial collections, not just columnar writes. The scan decodes the policy's ScanFilter payload once up front and rejects a statement outright when the payload is undecodable, rather than treating it as no policy. Matching rows are filtered by the query's WHERE predicates first, then the read policy, on flushed segments, the live memtable, and the in-transaction overlay alike, so excluded rows never consume a LIMIT slot or reach a join partner. The spatial scan handler splits from a single file into full_scan, prefilter, and rtree_scan modules to carry the new policy parameter alongside the existing full-scan and R-tree-prefiltered paths. Adds a wire test covering read-policy enforcement on columnar reads.
Row-level-security FOR READ policies now govern raw and grouped timeseries reads, matching the enforcement already in place for columnar and spatial. TimeseriesOp::Scan carries the caller's rls_filters payload through dispatch; the handler decodes it once up front and fails the statement outright on an undecodable payload rather than treating it as no policy. The raw scan evaluates the policy per row, after the WHERE predicates and before a row takes a LIMIT slot, on the live memtable, flushed partitions, and the in-transaction overlay alike. The aggregate path instead joins the policy onto the WHERE predicates and pushes the combined set into the grouped scan, so excluded rows never reach an accumulator; the COUNT(*) metadata fast path is skipped whenever a policy is present. eval_filters_to_bitmask and the aggregate_memtable/aggregate_partition callers now return Result instead of silently treating an unlowerable predicate as "no filter": a policy shape the grouped scan cannot express fails the aggregate rather than aggregating ungoverned rows. UnsupportedPredicate wires into the crate's central error type. Adds a wire test covering read-policy enforcement across the memtable and flushed-partition paths, with harness support for lowering the timeseries memtable budget so a test can force a flush.
Replace direct ShapedRows struct-literal construction across neutral DDL handlers with the from_json_rows/text_rows/with_notice helpers, removing repeated notice: None and column_types boilerplate at each call site.
Route response shaping through nodedb_types::Value end to end instead of serde_json::Value, so a cell keeps its typed form (instant, bytes, decimal, etc.) until a protocol renders it at its own edge. Adds decode_payload_value as the typed counterpart of decode_payload_to_json, and a single value_to_wire_json/row_to_wire_json conversion in util::wire_json + response_shape::cell that every protocol (pgwire, HTTP, native) and the redaction path now render through. Splits response_shape::compose and response_shape::types into directories (compose/kernel.rs, compose/materialized.rs, compose/array_slice.rs; types/plan_kind.rs, types/shaped.rs) to keep each concern in its own file as the row representation changes.
Timestamp and Timestamptz columns now go through coerce_to_instant alongside the existing Int64/Float64 coercion: a typed instant is retagged to the declared kind, ISO-8601 text is parsed, and a numeric literal is read as epoch milliseconds, the unit every engine's own ingest path already expects. A literal with no representable instant is refused at the statement, naming the column and the literal, and this is the exact result of taking the coercion path once instead of guessing at write or read time.
Splits handler/shape_encode.rs into a directory (cell.rs / response.rs / mod.rs) and adds a binary encode arm for Timestamp/Timestamptz, so a client requesting binary results gets PostgreSQL binary timestamp (microseconds since 2000-01-01) instead of a text-format downgrade. result_format::binary_supported and resolve_result_formats now honour that arm, and ddl_encode / response_shape::project route every cell through the one shared encoder instead of a local JSON-to-text match. response_shape::cell gains cell_text and instant_of, the typed readings a timestamp column renders a cell through, and response_shape::returning retypes announced-timestamp text into a typed instant (NaiveDateTime for TIMESTAMP, DateTime for TIMESTAMPTZ) instead of parsing it as an epoch integer. nodedb_types::NdbDateTime::parse now accepts a trailing UTC offset (+05:30, -0800, +02) in addition to Z, refuses partial or malformed spellings that the previous parser silently truncated, and both timestamp binary decoding and offset parsing are covered by new wire and unit tests.
Value::cmp_coerced silently ordered any unordered pair (an integer against an instant, text against a number, a NaN) as Equal, so a range predicate over such a pair matched every row instead of none. Replace it with partial_cmp_coerced, returning None for a pair with no defined order, the row-level counterpart of PostgreSQL refusing to compare two incompatible types. cmp_coerced remains for ORDER BY / MIN / MAX, where an unordered pair now maps explicitly to Equal so a stable sort keeps it in place. Also extends eq_coerced/ordering to compare a Decimal literal against a Float or Integer through the same numeric path as the existing Integer/Float/String coercions.
Splits the 500+ line file into a directory: expr_lower.rs (raw WHERE SqlExpr -> ScanFilter reduction) and serialize.rs (Filter tree -> ScanFilter msgpack encoding), with mod.rs re-exporting the entry points. No behavior change.
A literal in WHERE, ON, or WHEN compared against a declared TIMESTAMP / TIMESTAMPTZ column reached the planner exactly as written: a numeric literal stayed an untyped integer, and an instant column has no defined order or equality against an integer, so `=` never matched and `>=` matched nothing or matched by accident depending on the evaluator. predicate_coerce::coerce_predicate_literals walks the predicate tree (column <op> literal in either orientation, BETWEEN bounds, IN lists, through AND/OR/NOT) and resolves each literal against the column's declared type through the same coerce_value the write side already uses, so an instant column is always compared against an instant. A literal with no representable instant is refused at the statement, naming the column and the literal, instead of silently matching nothing. The primary-key column and every non-instant declared type are left untouched: the primary key derives identity from the literal's own rendering on both sides, and every other type is already covered by the Data Plane's coerced comparison. Wired into convert_where_to_filters (the one choke point for SELECT, UPDATE/DELETE, HAVING, LATERAL, and MERGE WHEN predicates) and into join ON post-filters. declared_type_coerce::coerce_value is exposed to the new module, and its type-mismatch messages are reworded to state what a literal is, not what a column can't store.
… kind FilterOp::parse_op (and the From<&str>/From<String> impls built on it) silently mapped any unrecognized wire tag to MatchAll, so a typo or a future operator name decoded as "match everything" instead of failing. Replace it with a fallible FilterOp::parse returning UnknownFilterOp, wired through serde deserialization and every msgpack/wire decode path. FilterOp also drops its Default impl (MatchAll is no longer a sentinel default), and value_ops::compare_values becomes partial_compare_values, returning None for a pair with no defined order per the new Value::partial_cmp_coerced, propagated through binary-op evaluation and graph pattern predicate checks. Adds TimeKind::literal_ms / cell_value (columnar_memtable::time_literal) as the one place a time column's stored millisecond count is lowered from, and read back to, the value its kind denotes: an Instant column reads a typed datetime or datetime text, a Millis column reads an integer, a truncated finite float, or datetime text. This replaces the removed scan_filter::value_as_timestamp_ms, which guessed a single unit for every time column regardless of declared kind, across the columnar filter evaluator, the timeseries time-range narrowing prefilter, grouped bitmask filtering, and scan materialization. Every ScanFilter constructed with a string op literal (`op: "eq".into()`) across the executor handlers, storage cold-filter pruning, and tests is updated to the typed FilterOp variant, and cold_filter's row-group pruning now matches on FilterOp directly with an exhaustive arm per operator instead of a serde_json::Value round-trip.
RLS policies were stored as pre-compiled predicate JSON (compiled_predicate_json), frozen at CREATE time: an ALTER COLLECTION that added, dropped, or renamed a column left every existing policy typed against the old schema, and a literal that failed to compile was silently skipped with a warn! log, so a broken policy quietly stopped restricting rows instead of refusing to install. StoredRlsPolicy now stores predicate_text (plus the database_id its declared columns live in) and recompiles it on every read through the single entry point compile_policy_predicate: parse, validate $auth.* references, then predicate_typing::type_predicate_literals types every literal a Compare node pairs with a declared TIMESTAMP/TIMESTAMPTZ column through nodedb_sql's coerce_read_literal, the same rule a query's WHERE literal follows, so a policy and a query on the same column agree on the instant. A literal the declared type cannot read is a compile error naming the column and the literal. StoredRlsPolicy::rehydrate (catalog/rls.rs, via rls/compile.rs) is the one path that turns stored text into a runtime RlsPolicy: on a compile failure it installs a restrictive deny-all instead of skipping the policy, so an unenforceable policy fails closed rather than silently admitting every row. Boot replay, Raft post-apply, and recovery reload all call it uniformly. recompile_for_collection reruns it for every policy on a collection whenever that collection's declared columns change (ALTER ADD COLUMN, strict schema changes, and the Raft post-apply of a collection catalog entry), keeping policies current with the schema without a restart. combined_read_predicate_with_auth and combined_write_predicate_with_auth now return a Result: encoding a compiled predicate to ScanFilter bytes can fail, and every caller (read-gate checks, EXPLAIN visibility, graph index creation, the planner's RLS injection) propagates that error instead of treating it as "no policy". predicate_eval.rs splits into a directory (filters.rs for the two constant ScanFilter shapes, sets.rs, substitute.rs) alongside the new predicate_typing.rs and rls/compile.rs.
DEFAULT materialization moves from a per-engine step in the control plane (sql_plan_convert::value::defaults::expand_row_defaults) into the planner itself: every write plan (Insert, Upsert, TimeseriesIngest) now carries rows with every declared DEFAULT already materialized and every literal already coerced to its column's declared type, plus a volatile_defaults flag replacing the old column_defaults text pairs so the plan cache still refuses a plan holding a fresh-per-row generator. Conversion no longer re-reads the catalog's DEFAULT text, and default_expr_is_volatile/defaults_are_volatile move out of volatility_scan into the planner's own defaults module. declared_type_coerce::coerce_write_literal is the new entry point a caller outside the planner uses for a literal that will reach storage under a column: it applies the same write-side coercion and range check a VALUES literal gets, exempting the primary key exactly as every VALUES/SET path does. The DDL gate (column_default.rs) now calls it for every column DEFAULT that spells a bare literal, so `DEFAULT 'not a date'` on a TIMESTAMP column and `DEFAULT 999999` on a SMALLINT column are refused at CREATE/ALTER instead of only surfacing once a row is inserted. CompiledDefault::literal() exposes the bare literal a DEFAULT spells; a generator or parsed expression has none to check yet. A refused literal DEFAULT raises the new sqlstate 42804 (datatype_mismatch) or the existing numeric-range SQLSTATE, mapped to BAD_REQUEST. catalog_adapter::declared_column_info centralizes the ColumnInfo a raw (name, type_str) DDL declaration resolves to, used both by the schemaless/columnar-family catalog arms and by the DDL gate, so a DEFAULT is judged against exactly the type its column will carry at INSERT time. planner/dml.rs and planner/dml_update_delete.rs split into a dml/ directory (insert, target, update_delete, upsert); planner/merge.rs splits into a merge/ directory (actions, plan) grouping the DML/MERGE planners with the write-side literal coercion they now perform.
Register a restored collection with the Data Plane before reissuing its rows, instead of relying on the catalog row alone: without the Data Plane declaration, a restored timeseries collection had no doc_configs entry, so reissued rows were ingested with an inferred shape, the declared time key as a plain integer, and the restore-time clock instead of the original timestamp. Carry a declared TIMESTAMP/TIMESTAMPTZ time key as a typed fixext8 instant on the timeseries ingest payload, rather than the ISO 8601 text used by other engines, and decode it on the Data Plane side back into the correct instant kind. Reject a time column whose stored value the line-protocol timestamp cannot carry, rather than silently dropping it or stamping the row with the ingest clock. Read a memtable or flushed-partition time cell back typed by its declared kind during restore reissue, returning an error for a millisecond count outside the instant's representable range.
farhan-syah
added
the
run-ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Index into scalars by reference instead of consuming the vector, and include the scalar's index in the assertion message so a failure identifies which case broke.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A
TIMESTAMPcell's unit was decided by whichever path read it. The timeseries engine stores milliseconds; four Data Plane converters rescaled to microseconds on the way out, each keyed by a column-name list. A path without a converter emitted milliseconds, and the pgwire encoder read any number under aTIMESTAMPcolumn as microseconds. A cross-node join compared a gathered remote side (microseconds) against a local side (milliseconds) and dropped every row; atime_bucketin a SELECT list saw milliseconds directly and microseconds through a join; strictINSERT ... RETURNINGrendered 1970. The same flaw held for literals: an integer compared against an instant column was milliseconds in one pushdown, unordered on the row path, and never coerced on write or in a policy.Closes #317
What changes
One typed instant, carried by the cell, from storage to every protocol edge.
1UTC /2naive,i64epoch microseconds;nodedb_types::{InstantKind, write_instant, read_instant};Value::DateTime/NaiveDateTimeon both sidesColumnType::Timestamp(TimeKind),TimeKind::{Millis, Instant(InstantKind)}; storage staysi64ms, the emitter matches the kindcolumnar_read/convert.rs write_time_cell,TimeKind::cell_value,nodedb_types::columnar::ColumnType::time_cell— the four rescalers,instant_scale.rs,JoinParams.instant_columns, andts_instant_columnsare deletedRowsPayload.rows: Vec<Vec<NativeCell>>— typedValue, not pre-rendered textShapedRow = BTreeMap<String, Value>;util/wire_json.rs value_to_wire_jsonis the one JSON edge; pgwireshape_encode/cell.rs encode_cell(Value, DdlColType)nodedb-sql declared_type_coerce::coerce_to_instant: text is parsed, a number is epoch milliseconds, other kinds are refused naming the columnnodedb-sql planner/predicate_coerce.rsapplies the same rule toWHERE/ON/BETWEEN/IN/ UPDATE / DELETE;TimeKind::literal_mslowers to the storage unit in both pushdownsScanFilter.valuecrosses as the taggedValue, not JSON;FilterOp::parseis fallibletime_bucket,date_part,date_trunc,datetime,date_add/date_sub,date_diffaccept and return typed instantsBehaviour changes
ONcomparing two time keysONbetween a timeseries time key and adocument_strictTIMESTAMPtime_bucket(...)in a SELECT list, groupedbucketcolumndate_part('year', ts)on a timeseries time keyINSERT ... RETURNING created_atTIMESTAMPfrom a flushed segment vs the live memtableTIMESTAMP/TIMESTAMPTZTIMESTAMPcellValue::StringorValue::Integerby engineValue::NaiveDateTime/Value::DateTimeTIMESTAMPcolumn at the pgwire edgeWHERE ts >= <integer>on the row pathcmp_coercedansweredEqual)WHERE f64_col = $1with a boundFLOAT8Decimal)Decimalis a number in coerced comparisonNdbDateTime::parseof+05:30/ZoffsetsDEFAULT <literal>a column cannot holdCREATE, refused at INSERT (kv) or stored (others)CREATE/ALTER ADD COLUMN,42804/22003DEFAULTTIME_KEY DEFAULTis the row's time keyINSERT VALUES/UPDATE SETWHEREthe bitmask evaluator cannot lowerFixed on the way
FOR READpolicies now govern columnar, spatial, and timeseries scans, aggregates, joins,LIMIT, and the in-transaction overlay. Policy literals are typed against the declared column atCREATE RLS POLICYand recompiled from stored text on load and afterALTER. A policy that cannot compile or serialize installs deny-all, never admit-all.INTERSECTSemitted an operator name the wire decoder mapped to match-all; an unknown operator is now a decode error.Valueconverter (util/rmpv_value.rs) replaces four; one msgpack →Valuedecoder replaces two.limit * 10raw-row cap that silently truncated filtered columnar results is removed.json_msgpack/reader/,msgpack_scan/reader/,columnar_memtable/memtable/,grouped_scan/strategies/,columnar_read/scan/,handlers/spatial/,response_shape/{compose,types}/,pgwire/handler/shape_encode/,sql_plan_convert/filter/,security/predicate_eval/,planner/{dml,merge}/.Compatibility
Pre-1.0, no migration:
Value::DateTime/NaiveDateTimeis fixext8, not an ISO string. Every reader in this workspace is updated;nodedb-litemust mirror it.instant_naive/instant_utc/timestamp. Memtable snapshot layout ofColumnTypechanged. A partition written before this reads back asMillis.RowsPayload,ScanFilter,ShapedRows,StoredRlsPolicy(storespredicate_text, not a compiled predicate),SqlPlan::Insert/Upsert(volatile_defaults, nocolumn_defaults), andFilterOp(noFrom<&str>) changed shape.TIMESTAMPcolumns written as millisecond text no longer match.EXTRACT-style scalars accept typed instants; an integer literal against an instant column is epoch milliseconds everywhere.Unrelated defect found while tracing: #333 (timeseries aggregates drop what the native scan cannot express).