rust/greptimedb
1
0
Fork 0
mirror of https://github.com/GreptimeTeam/greptimedb.git synced 2026-11-14 06:46:56 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: drop trivial test cases and shorten comments

Browse Source 
Review pass over the branch's additions:

- datatypes: keep a representative subset of the widening-matrix asserts
- table: collapse the three single-branch rejection blocks into one loop
- mito2: drop the boundary gt_eq and post-compaction predicate asserts
  (covered by the exact-filter regression test and sqlness); drop the
  engine-level gt_eq/lt_eq casts (full operator matrix stays in the
  cast_timestamp_unit unit tests)
- sqlness: drop a bare full scan already covered by the filter above it
- shorten function doc comments across datatypes/store-api/table/mito2/
  recordbatch to the essential semantics

Signed-off-by: Ning Sun <sunning@greptime.com>
This commit is contained in:
Ning Sun
2026-08-26 16:55:39 +08:00
parent a9a1d9de84
commit 3cfdf52e0a
11 changed files with 92 additions and 230 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/common/recordbatch/src/filter.rs
+14 -22
View File
@@ -342,24 +342,17 @@ impl SimpleFilterEvaluator {
}
/// Casts the filter's timestamp literal into the unit of the `target`
/// timestamp type, so the filter can be evaluated directly against a
/// column stored in `target` (e.g. an old-unit SST after the time index
/// unit was widened) instead of being skipped for a cross-type
/// comparison error.
/// timestamp type, so it can be evaluated against a column stored in that
/// unit (e.g. an old-unit SST after the time index unit was widened).
///
/// The conversion preserves the predicate's semantics on rows of the
/// target unit exactly. When the literal is not representable in the
/// target unit (e.g. `ts = 7_000_500us` against a millisecond column),
/// the outcome keeps the row set instead of rounding the literal:
/// - `=` reports [`TimestampUnitCast::Pruned`] (no row can match) and
/// `!=` reports [`TimestampUnitCast::Matched`] (every row matches);
/// - `>=` / `<` strengthen the operator (`>= 2_500_500us` on a
/// millisecond column becomes `> 2500ms`) so the boundary row the
/// rounded literal would wrongly include stays excluded.
/// When the literal is not representable in `target`'s unit (e.g.
/// `ts = 7_000_500us` against a millisecond column), the outcome keeps the
/// row set instead of rounding the literal: `=` prunes / `!=` matches, and
/// inequalities strengthen the operator (e.g. `>= 2_500_500us` becomes
/// `> 2500ms`) so the excluded boundary row stays excluded.
///
/// Returns `None` when the filter doesn't compare against a tz-naive
/// timestamp literal, or `target` is not a timestamp type; the caller
/// decides how to treat such filters.
/// timestamp literal, or `target` is not a timestamp type.
pub fn cast_timestamp_unit(&self, target: &ConcreteDataType) -> Option<TimestampUnitCast> {
let target_unit = match target.as_arrow_type() {
DataType::Timestamp(unit, _) => unit,
@@ -488,18 +481,17 @@ fn timestamp_scalar(value: i64, unit: TimeUnit) -> Option<Scalar<ArrayRef>> {
scalar.to_scalar().ok()
}
/// The exact division of a literal's instant into the target unit.
/// The exact division of a literal's instant into the target unit:
/// `quotient * literal_scale + remainder == literal * target_scale` with
/// `0 <= remainder < literal_scale`. `remainder == 0` iff the literal is
/// representable in the target unit.
struct UnitQuotient {
/// `quotient * literal_scale + remainder == literal * target_scale`, with
/// `0 <= remainder < literal_scale`. `quotient` is the literal's value
/// rounded toward negative infinity in the target unit, and `remainder`
/// is zero iff the literal is representable in the target unit.
quotient: i64,
remainder: i128,
}
/// Divides a `literal` (in `literal_unit`) by the `target_unit`, i.e. computes
/// its value in `target_unit` exactly.
/// Divides a `literal` (in `literal_unit`) by the `target_unit`, i.e.
/// computes its value in `target_unit` exactly (floor division).
fn div_mod_units(
literal: i64,
literal_unit: TimeUnit,
src/datatypes/src/data_type.rs
+7 -18
View File
@@ -375,14 +375,10 @@ impl ConcreteDataType {
}
}
/// Checks whether changing a timestamp column from this type to `to_type` is a
/// **widening unit change**: both types are timestamps and the target unit is
/// strictly finer than the source unit (e.g. `Second -> Millisecond`).
///
/// Such a change is lossless for all values that fit the target unit's `i64`
/// range (note that `Millisecond/Microsecond -> Nanosecond` overflows for
/// timestamps beyond year 2262). The reverse (narrowing) direction truncates
/// values and is therefore rejected by callers of this check.
/// Returns true if both types are timestamps and `to_type`'s unit is strictly
/// finer (e.g. `Second -> Millisecond`). Such a change is lossless for
/// values that fit the target unit's `i64` range; the narrowing direction
/// truncates values.
pub fn is_timestamp_unit_widening_to(&self, to_type: &ConcreteDataType) -> bool {
match (self.as_timestamp(), to_type.as_timestamp()) {
(Some(from), Some(to)) => to.unit() > from.unit(),
@@ -744,23 +740,16 @@ mod tests {
fn test_is_timestamp_unit_widening_to() {
let second = ConcreteDataType::timestamp_second_datatype();
let milli = ConcreteDataType::timestamp_millisecond_datatype();
let micro = ConcreteDataType::timestamp_microsecond_datatype();
let nano = ConcreteDataType::timestamp_nanosecond_datatype();
assert!(second.is_timestamp_unit_widening_to(&milli));
assert!(milli.is_timestamp_unit_widening_to(&micro));
assert!(micro.is_timestamp_unit_widening_to(&nano));
assert!(second.is_timestamp_unit_widening_to(&nano));
// Narrowing and same-unit changes are not widening.
assert!(!nano.is_timestamp_unit_widening_to(&micro));
// Same unit and narrowing are not widening.
assert!(!milli.is_timestamp_unit_widening_to(&milli));
assert!(!second.is_timestamp_unit_widening_to(&second));
// Non-timestamp types are never involved.
assert!(!nano.is_timestamp_unit_widening_to(&milli));
// Either side being a non-timestamp type disqualifies.
assert!(!milli.is_timestamp_unit_widening_to(&ConcreteDataType::int64_datatype()));
assert!(!ConcreteDataType::int64_datatype().is_timestamp_unit_widening_to(&nano));
assert!(!ConcreteDataType::string_datatype().is_timestamp_unit_widening_to(&milli));
}
#[test]
src/datatypes/src/error.rs
+2 -7
View File
@@ -21,13 +21,8 @@ use snafu::{Location, Snafu};
use crate::data_type::ConcreteDataType;
/// Formats the shared error message used when a time index column's type
/// change is rejected because it is not a widening timestamp unit change
/// (see [`ConcreteDataType::is_timestamp_unit_widening_to`]).
///
/// Both the region layer (`store-api`) and the table layer (`table`) validate
/// this rule; sharing one message keeps the two validations from drifting
/// apart.
/// Shared error message for rejecting a time index type change that is not a
/// widening timestamp unit change; used by both the region and table layer.
pub fn time_index_not_widening_error(
column_name: &str,
from_type: &ConcreteDataType,
src/mito2/src/engine/alter_test.rs
+22 -74
View File
@@ -620,16 +620,10 @@ fn ts_us_lit(v: i64) -> datafusion_expr::Expr {
lit(ScalarValue::TimestampMicrosecond(Some(v), None))
}
/// Asserts every `expected` row is present in `actual` (with its exact
/// value).
///
/// `ScanRequest.filters` are an advisory pushdown for most columns, so this
/// helper only asserts the engine's pruning guarantee: no file or row group
/// holding matching rows is wrongly pruned — i.e. matching rows are never
/// lost. Exact row-level filtering of old-unit SSTs after widening the time
/// index is asserted by
/// [`test_alter_time_index_widen_old_sst_filters_rows_exactly`]; other exact
/// filtering is covered by the sqlness cases.
/// Asserts every `expected` row is present in `actual`: `ScanRequest.filters`
/// are advisory for pruning, so the guarantee under test is that no matching
/// row is lost. Exact filtering is asserted by
/// [`test_alter_time_index_widen_old_sst_filters_rows_exactly`].
fn assert_rows_present(expected: &[(i64, f64)], actual: Vec<(i64, f64)>) {
for row in expected {
assert!(
@@ -641,15 +635,9 @@ fn assert_rows_present(expected: &[(i64, f64)], actual: Vec<(i64, f64)>) {
}
}
/// Verifies the read paths over old-unit (pre-alter) SST data after widening
/// the time index unit:
/// - predicate scans (file-level and row-group pruning see file stats in the
/// old unit while the predicate is in the new unit),
/// - dedup when a post-alter write overwrites a pre-alter row at the same
/// instant (the old row must be cast to the new unit *before* dedup),
/// - mixed-unit SSTs (old files + a flushed post-alter file),
/// - compaction, which reads old-unit files through the compat layer and
/// rewrites them in the new unit.
/// Read paths over old-unit (pre-alter) SST data after widening the time
/// index unit: predicate scans, dedup across units, mixed-unit SSTs, and
/// compaction rewriting old-unit files in the new unit.
#[tokio::test]
async fn test_alter_time_index_read_paths_after_widen() {
common_telemetry::init_default_ut_logging();
@@ -721,9 +709,8 @@ async fn test_alter_time_index_read_paths_after_widen() {
);
// Predicate scans: the predicate is in the new unit while the SST stats
// are in the old unit. No row-matching file/row-group may be pruned —
// every matching row stays present with its original value. (Exact
// filtering is the query layer's residual filter; see sqlness.)
// are in the old unit; no file or row group holding a matching row may be
// pruned. (Exact filtering is asserted by the test below and sqlness.)
assert_rows_present(
&[(3_000_000, 3.0), (4_000_000, 4.0)],
scan_ts_and_field(&engine, region_id, vec![col("ts").gt(ts_us_lit(2_500_000))]).await,
@@ -732,16 +719,6 @@ async fn test_alter_time_index_read_paths_after_widen() {
&[(0, 0.0), (1_000_000, 1.0)],
scan_ts_and_field(&engine, region_id, vec![col("ts").lt(ts_us_lit(1_500_000))]).await,
);
// Boundary predicate at exactly a stored timestamp.
assert_rows_present(
&[(2_000_000, 2.0), (3_000_000, 3.0), (4_000_000, 4.0)],
scan_ts_and_field(
&engine,
region_id,
vec![col("ts").gt_eq(ts_us_lit(2_000_000))],
)
.await,
);
// A post-alter write overwrites the pre-alter row ("2", ts = 2s) with a new
// field value, and inserts a sub-millisecond row at 5.5s. The old row and
@@ -794,8 +771,8 @@ async fn test_alter_time_index_read_paths_after_widen() {
// Flush the post-alter writes so the region holds both an old-unit SST
// and a new-unit SST, then compact. Compaction reads the old-unit file
// through the compat layer and rewrites everything in the new unit; all
// values (including the deduped overwrite) must survive.
// through the compat layer and rewrites it in the new unit; all values
// (including the deduped overwrite) must survive.
flush_region(&engine, region_id, None).await;
assert_eq!(
expected_after_overwrite,
@@ -813,28 +790,15 @@ async fn test_alter_time_index_read_paths_after_widen() {
expected_after_overwrite,
scan_ts_and_field(&engine, region_id, vec![]).await
);
assert_rows_present(
&[(3_000_000, 3.0), (4_000_000, 4.0), (5_500_000, 5.5)],
scan_ts_and_field(&engine, region_id, vec![col("ts").gt(ts_us_lit(2_500_000))]).await,
);
}
/// Verifies that predicates on the time index filter old-unit (pre-alter)
/// SST rows **exactly** after widening the time index unit (regression test
/// for the over-inclusive scans reported in testbed/issue.md: the predicate
/// used to be dropped for old-unit files, returning every row of each
/// surviving row group).
///
/// The fixture uses two row groups of five rows so exact-count assertions
/// distinguish all failure modes:
/// - the exact row set — correct (row filter applied)
/// - 5 rows (ts = 5s..9s) — row-group pruning worked, row filter dropped (the bug)
/// - 0 rows — over-pruning
/// - 10 rows — no pruning at all
///
/// Also covers literals that are not representable in the file's (ms) unit
/// (`ts > 6_500_000us`): the cast filter must strengthen the operator
/// instead of rounding the literal, and `!=` must keep complementing `=`.
/// Regression test: predicates on the time index must filter old-unit
/// (pre-alter) SST rows **exactly** after widening — the predicate used to be
/// dropped for old-unit files, returning every row of each surviving row
/// group. Two row groups of five rows make a partial-match result
/// distinguishable from both over- and under-selection; literals not
/// representable in the file's (ms) unit must strengthen the operator
/// instead of rounding to a boundary row.
#[tokio::test]
async fn test_alter_time_index_widen_old_sst_filters_rows_exactly() {
common_telemetry::init_default_ut_logging();
@@ -921,8 +885,10 @@ async fn test_alter_time_index_widen_old_sst_filters_rows_exactly() {
assert_eq!(expected_ne, actual_ne);
// Literals not representable in the file's millisecond unit: the cast
// filter must strengthen the operator (>, >=, <, <= around 6.5s) instead
// of rounding the literal to a matching boundary row (6s or 7s).
// filter must strengthen the operator instead of rounding the literal to
// a boundary row (6s or 7s). `gt` and `lt` are the engine-level
// representatives; the full operator matrix is covered by the unit tests
// of `cast_timestamp_unit`.
let rows_after = |from: i64| -> Vec<(i64, f64)> {
(from..10).map(|ts| (ts * 1_000_000, ts as f64)).collect()
};
@@ -932,28 +898,10 @@ async fn test_alter_time_index_widen_old_sst_filters_rows_exactly() {
rows_after(7),
scan_ts_and_field(&engine, region_id, vec![col("ts").gt(ts_us_lit(6_500_000))]).await
);
assert_eq!(
rows_after(7),
scan_ts_and_field(
&engine,
region_id,
vec![col("ts").gt_eq(ts_us_lit(6_500_000))]
)
.await
);
assert_eq!(
rows_before(7),
scan_ts_and_field(&engine, region_id, vec![col("ts").lt(ts_us_lit(6_500_000))]).await
);
assert_eq!(
rows_before(7),
scan_ts_and_field(
&engine,
region_id,
vec![col("ts").lt_eq(ts_us_lit(6_500_000))]
)
.await
);
// A literal that no millisecond row can equal must not match anything.
assert!(
src/mito2/src/sst/parquet/reader.rs
+10 -16
View File
@@ -2059,22 +2059,16 @@ impl SimpleFilterContext {
{
MaybeFilter::Filter(filter)
} else {
// Schema evolution can make columns with the same id
// have different concrete data types across SSTs:
// altering a field column's type, or widening the
// time index unit. Evaluating the original filter
// against such an SST may raise an invalid
// cross-type comparison error (e.g. Float64 == Utf8,
// or a microsecond literal vs a millisecond column).
//
// Timestamp predicates must remain exactly enforced
// here: the scan pipeline does not re-apply them to
// the compat-cast batches of this SST, so cast the
// predicate into the file's timestamp unit instead of
// dropping it. Other mismatched types (altered field
// columns) keep the conservative skip: the query
// layer re-applies those predicates above the region
// scan.
// Schema evolution (altered field columns, or a
// widened time index unit) can make columns with
// the same id have different types across SSTs;
// evaluating the original filter may raise an
// invalid cross-type comparison. Timestamp
// predicates are not re-applied above this scan,
// so cast them into the file's unit instead of
// dropping them; other mismatches keep the
// conservative skip (the query layer re-applies
// those predicates).
match filter.cast_timestamp_unit(&sst_column.column_schema.data_type) {
Some(TimestampUnitCast::Filter(filter)) => {
MaybeFilter::Filter(filter)
src/mito2/src/sst/parquet/stats.rs
+7 -12
View File
@@ -81,19 +81,14 @@ impl<'a, T> RowGroupPruningStats<'a, T> {
/// Casts stats values to the expected data type when the SST stores the
/// column with a different type (e.g. after `MODIFY COLUMN` changed the
/// column type, including widening the time index unit).
/// column type, including widening the time index unit). The pruning
/// predicate is built against the expected schema, so raw file-typed
/// stats would prune wrongly.
///
/// The pruning predicate is built against the expected (current) schema,
/// so comparing raw file-typed stats against it can prune wrongly (e.g.
/// `4_000` milliseconds max vs a `2_500_000` microseconds literal).
///
/// Parquet statistics of a timestamp column are raw integers, so the cast
/// is done in two steps: first interpret them in the file's type (a plain
/// reinterpret, e.g. `Int64 -> Timestamp(ms)`), then convert to the
/// expected type (a unit-aware conversion, e.g. `ms -> us`). A single-step
/// `Int64 -> Timestamp(us)` would reinterpret the value as microseconds
/// instead of scaling it. Returns `None` when either cast fails, so the
/// row group is kept (conservative).
/// Timestamp stats are raw integers: first reinterpret them in the file's
/// type, then convert to the expected type. A single-step cast would
/// reinterpret (not rescale) the value. Returns `None` on cast failure so
/// the row group is kept (conservative).
fn cast_stats_to_expected(&self, column_id: ColumnId, values: ArrayRef) -> Option<ArrayRef> {
// Without expected metadata the file metadata is the expected one,
// so there is nothing to cast toward.
src/mito2/src/worker/handle_alter.rs
+4 -8
View File
@@ -462,14 +462,10 @@ fn log_option_update<T: std::fmt::Debug>(
);
}
/// Checks that widening the time index unit does not overflow existing data.
///
/// The region's memtables are empty when this runs (they are flushed before an
/// alter), so all data lives in SSTs and each file's time range bounds the
/// values it stores. Widening casts multiply values by the unit factor, and
/// values that no longer fit `i64` would be cast to NULL by Arrow — while the
/// time index column is NOT NULL. So reject the alter if any file's time
/// range overflows the target unit.
/// Rejects a time index unit widening whose rescaled values would overflow
/// `i64` in existing data. Memtables are flushed before an alter, so each
/// SST's time range bounds its values; Arrow would cast overflowing values
/// to NULL, which a NOT NULL time index cannot hold.
fn check_time_index_widening_overflow(
region_id: RegionId,
version: &VersionRef,
src/store-api/src/region_request.rs
+6 -13
View File
@@ -1414,15 +1414,10 @@ impl ModifyColumnType {
}
);
}
// The time index column only supports widening timestamp unit changes
// (e.g. `TimestampMillisecond -> TimestampMicrosecond`), which are
// lossless. Readers cast historical data in old SSTs to the new unit
// on the fly. Narrowing changes truncate values and are rejected.
//
// A same-type change is accepted here so that a retried alter
// procedure (region already altered before a previous attempt
// failed) validates and is skipped as a no-op by `need_alter`,
// instead of failing the retry forever.
// The time index column only supports widening its timestamp
// unit; historical SST data is cast to the new unit on read.
// A same-type change validates so a retried alter procedure is a
// no-op instead of failing the retry forever.
SemanticType::Timestamp => {
ensure!(
column_meta.column_schema.data_type == self.target_type
@@ -1456,10 +1451,8 @@ impl ModifyColumnType {
}
/// Returns true if no column's datatype to change to the region.
///
/// A column whose data type already equals the target type needs no
/// alteration. This keeps a retried alter (e.g. a procedure resubmitting
/// after the region was already altered) a successful no-op.
/// A column already in the target type needs no alteration, so a retried
/// alter is a successful no-op.
pub fn need_alter(&self, metadata: &RegionMetadata) -> bool {
debug_assert!(self.validate(metadata).is_ok());
metadata
src/table/src/metadata.rs
+18 -44
View File
@@ -1048,10 +1048,9 @@ impl TableMeta {
let is_time_index = timestamp_index == Some(index);
if is_time_index {
// The time index column is NOT NULL by construction and its unit
// only supports lossless widening changes (e.g. millisecond ->
// microsecond). Historical data in SSTs is cast to the new unit
// on read, so no backfill is needed.
// The time index column is NOT NULL by construction and only
// supports widening its unit; historical data in SSTs is cast
// to the new unit on read, so no backfill is needed.
ensure!(
column
.data_type
@@ -1707,47 +1706,22 @@ mod tests {
assert_eq!(new_meta.schema.version(), 124);
assert_eq!(&[0], &new_meta.primary_key_indices[..]);
// Narrowing is rejected.
let alter_kind = AlterKind::ModifyColumnTypes {
columns: vec![ModifyColumnTypeRequest {
column_name: "ts".to_string(),
target_type: ConcreteDataType::timestamp_second_datatype(),
}],
};
let err = {
// Any non-widening change (narrowing, same type, non-timestamp) is
// rejected.
for target in [
ConcreteDataType::timestamp_second_datatype(),
ConcreteDataType::timestamp_millisecond_datatype(),
ConcreteDataType::string_datatype(),
] {
let alter_kind = AlterKind::ModifyColumnTypes {
columns: vec![ModifyColumnTypeRequest {
column_name: "ts".to_string(),
target_type: target.clone(),
}],
};
let res = meta.builder_with_alter_kind("my_table", &alter_kind);
assert!(res.is_err());
// `TableMetaBuilder` doesn't implement `Debug` so we can't use `unwrap_err()`.
match res {
Err(e) => e,
Ok(_) => unreachable!(),
}
};
assert!(err.to_string().contains("only supports widening"), "{err}");
// Changing to the same type is rejected.
let alter_kind = AlterKind::ModifyColumnTypes {
columns: vec![ModifyColumnTypeRequest {
column_name: "ts".to_string(),
target_type: ConcreteDataType::timestamp_millisecond_datatype(),
}],
};
assert!(
meta.builder_with_alter_kind("my_table", &alter_kind)
.is_err()
);
// Changing to a non-timestamp type is rejected.
let alter_kind = AlterKind::ModifyColumnTypes {
columns: vec![ModifyColumnTypeRequest {
column_name: "ts".to_string(),
target_type: ConcreteDataType::string_datatype(),
}],
};
assert!(
meta.builder_with_alter_kind("my_table", &alter_kind)
.is_err()
);
assert!(res.is_err(), "expected rejection for {target}");
}
}
#[test]
tests/cases/standalone/common/alter/change_col_type.result
+1 -12
View File
@@ -159,15 +159,6 @@ SELECT * FROM ts_widen WHERE ts > '2024-01-01 00:00:00.999999' ORDER BY ts;
| b | 2024-01-01T00:00:01.000500 |
+------+----------------------------+
SELECT * FROM ts_widen ORDER BY ts;
+------+----------------------------+
| host | ts |
+------+----------------------------+
| a | 2024-01-01T00:00:01 |
| b | 2024-01-01T00:00:01.000500 |
+------+----------------------------+
-- narrowing the time index unit is rejected
ALTER TABLE ts_widen MODIFY COLUMN ts TIMESTAMP;
@@ -183,9 +174,7 @@ DROP TABLE ts_widen;
Affected Rows: 0
-- Regression: predicates must filter old-unit SST rows exactly after the
-- widen. The fixture stores ten rows in one row group (one SST, flushed in
-- the old millisecond unit), so any predicate that matches only a subset of
-- the rows exposes an over-inclusive scan that returns whole row groups.
-- widen; a dropped filter would return whole row groups.
CREATE TABLE ts_widen_multi (host STRING, ts TIMESTAMP TIME INDEX);
Affected Rows: 0
tests/cases/standalone/common/alter/change_col_type.sql
+1 -4
View File
@@ -52,7 +52,6 @@ INSERT INTO ts_widen VALUES ("b", "2024-01-01 00:00:01.000500");
SELECT * FROM ts_widen WHERE ts > '2024-01-01 00:00:01.000001' ORDER BY ts;
SELECT * FROM ts_widen WHERE ts > '2024-01-01 00:00:00.999999' ORDER BY ts;
SELECT * FROM ts_widen ORDER BY ts;
-- narrowing the time index unit is rejected
ALTER TABLE ts_widen MODIFY COLUMN ts TIMESTAMP;
@@ -63,9 +62,7 @@ ALTER TABLE ts_widen MODIFY COLUMN ts STRING;
DROP TABLE ts_widen;
-- Regression: predicates must filter old-unit SST rows exactly after the
-- widen. The fixture stores ten rows in one row group (one SST, flushed in
-- the old millisecond unit), so any predicate that matches only a subset of
-- the rows exposes an over-inclusive scan that returns whole row groups.
-- widen; a dropped filter would return whole row groups.
CREATE TABLE ts_widen_multi (host STRING, ts TIMESTAMP TIME INDEX);
INSERT INTO ts_widen_multi VALUES
("a", "2024-01-01 00:00:00"), ("b", "2024-01-01 00:00:01"),