Timeseries model

[lvca]

New Timeseries module, finally!

[mergify]

🧪 CI Insights

Here's what we observed from your CI run for 02beced.

🟢 All jobs passed!

But CI Insights is watching 👀

[claude]

Code Review: Timeseries Module

This is an impressive and substantial addition. The architecture is well thought out — columnar storage, crash-safe compaction, Gorilla XOR + delta-of-delta codecs, SIMD acceleration with scalar fallback, push-down aggregation, PromQL support, and continuous aggregates. The 26 test classes provide solid coverage. The following are issues worth addressing before merge.

---

Bugs / Correctness

1. CURRENT_VERSION = 0 — no upgrade path

Both TimeSeriesSealedStore.CURRENT_VERSION = 0 and the mutable bucket's format version use 0. LocalTimeSeriesType.fromJSON() throws IllegalStateException on version mismatch, meaning any future format change will hard-break existing databases with no migration path. At minimum, add a comment documenting how version upgrades must be handled, or add a skeleton migration dispatcher now while the version is still 0.

2. LineProtocolParser — silent data loss on malformed input

parseLine() returns null on parse errors; callers log a warning and skip the sample. This means a malformed line in a batch silently drops the entire point with no indication to the client. Consider either:

• throwing a LineProtocolException and letting callers choose to skip-or-fail, or
• returning a richer result type that distinguishes "parsed" from "parse error".

3. GorillaXORCodec — stale leading/trailing zero reuse

The Gorilla paper's "same leading/trailing zeros as previous" optimization is correct only when the block length is exactly the same. The canonical implementation tracks prevLeadingZeros and prevTrailingZeros and reuses them only when the new XOR fits within the same window. Please verify the codec handles the edge case where the new XOR has more leading zeros than the previous value — it should still use the longer form ('11'), not the reuse form ('10').

4. TimeSeriesMaintenanceScheduler — WeakReference TOCTOU

Database db = dbRef.get();
if (db == null) { task.cancel(false); return; }
// ... use db later ...

Between the null check and subsequent use, the referent could theoretically be GC'd (though in practice the ScheduledExecutorService keeps a strong ref to the Runnable, not to db). The pattern is fine for memory-leak avoidance, but the comment should be explicit that db is strongly reachable as long as task is running.

5. ContinuousAggregateRefresher.buildFilteredQuery() — WHERE injection fragility

The method finds the WHERE clause by scanning for parentheses and quoted strings, then string-splices in AND ts >= <watermark>. This is brittle: it will produce invalid SQL if the user query uses CTEs, sub-selects, or HAVING clauses. Consider requiring that continuous-aggregate queries be simple SELECT ... FROM <tsType> WHERE ... forms and validating that structure at CREATE CONTINUOUS AGGREGATE time rather than at refresh time.

---

Performance

6. loadDirectory() linear scan on open

When the sealed store is opened cold, loadDirectory() reads every block header sequentially to rebuild the in-memory block directory. For a large store (e.g., millions of blocks), startup time could be significant. Consider persisting a compact block-directory footer at the end of the sealed file, similar to Parquet's footer, so recovery is O(1) instead of O(N blocks).

7. Round-robin shard routing distributes time-ordered data across all shards

appendCounter.getAndIncrement() % shardCount means samples from a single sensor are distributed round-robin across N shards. Every time-range query must merge N iterators even for a single-source workload. For multi-tenant / multi-metric deployments this is fine, but for single-series high-frequency ingestion it doubles or quadruples I/O. Consider exposing an optional shardKey parameter on appendSamples() that routes by tag-hash so related time-series land on the same shard.

8. TimeSeriesEngine.close() — 30s hard timeout

shardExecutor.awaitTermination(30, TimeUnit.SECONDS);

On a clean shutdown (no pending tasks), this waits up to 30 seconds unnecessarily if shutdownNow() is not called. The pattern should be shutdown() followed by awaitTermination(), with a shutdownNow() if the timeout is reached. The current code only calls shutdown() and waits.

---

Security

9. jdk.incubator.vector — missing --add-modules JVM flag

SimdTimeSeriesVectorOps.java imports from jdk.incubator.vector, which requires --add-modules jdk.incubator.vector at both compile time and runtime on Java 21. This flag is absent from engine/pom.xml (checked: no argLine, no compiler compilerArgs). The TimeSeriesVectorOpsProvider does fall back to scalar on LinkageError, which saves runtime, but the compilation may silently succeed only because jvector brings the module transitively. Add explicit --add-modules jdk.incubator.vector to Surefire argLine and the compiler plugin to make the dependency explicit and avoid surprises in environments where jvector is absent or shaded.

10. sanitizeTypeName() in PromQLEvaluator — silent name collision

// Maps '.', '-', ':' to '_'

Two distinct ArcadeDB type names that differ only in those characters (e.g., cpu.load and cpu_load) will map to the same PromQL metric name. This should at minimum log a warning at the point where the collision is detectable (in sanitizeTypeName()), and ideally the sanitization should use a reversible bijection or raise an error for ambiguous names.

---

Code Quality / Minor

11. DownsamplingTier density threshold is a magic constant

if (sampleCount <= maxSamplesAfterDownsample * 0.9)
    continue; // already downsampled

The 90% threshold is undocumented. Extract it as a named constant (DOWNSAMPLE_DENSITY_THRESHOLD = 0.9) and add a comment explaining the trade-off (idempotency margin vs. precision loss).

12. FetchFromTimeSeriesStep.syncPull() — ResultSet.close() is a no-op

If the iterator holds open a file channel (e.g., via iterateRange()), the no-op close() on the inner ResultSet means the channel is not released until GC. Make close() delegate to the iterator's close() if the iterator implements Closeable.

13. AggregateFromTimeSeriesStep — missing tagFilter in copy()

Verify that copy() passes tagFilter when constructing the new instance. Omitting it would cause execution plan copies (used in distributed query re-planning) to silently drop tag-level predicates.

14. TimeSeriesBucket.clearDataPages() contract

The O(1) clear relies on the fact that the header's active-page counter is the sole truth about live data. This invariant must survive crash recovery. Please add a comment (or an assertion) documenting this, and verify that the mutable-bucket crash-recovery path in TimeSeriesShard always reads the header before trusting page content.

---

Positive highlights

• The 4-phase lock-free compaction protocol in TimeSeriesShard is elegant and correctly bounds the critical-section to header flips only.
• Block-level aggregation stats fast path (skip decompression when a block fits in one time bucket) is a great optimization.
• The ReDoS guard in PromQLEvaluator (pattern length cap + nested-quantifier rejection) is appropriate for a database that accepts user-supplied regex.
• WeakReference for database and type in the maintenance scheduler correctly avoids holding lifecycle references.
• The scalar/SIMD split with smoke-test fallback in TimeSeriesVectorOpsProvider is the right pattern.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -9.05%	✅ 79.79%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (5d7e12f)	93303	68696	73.63%
Head commit (1093ac1)	130522 (+37219)	84292 (+15596)	64.58% (-9.05%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3511)	6715	5358	79.79%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[claude]

Code Review: Timeseries Module

Great addition — the module is well-structured with a clear separation between storage (sealed store, buckets), codec, query execution, and schema layers. The test coverage is broad. The notes below are intended to help harden the implementation before merging.

---

HIGH severity

1. TimeSeriesBucket — silent data corruption via readShort sign extension

engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesBucket.java

final int sampleCountInPage = dataPage.readShort(DATA_SAMPLE_COUNT_OFFSET);

readShort returns a signed short (max 32 767). When the count reaches 32 767 and increments, the short wraps to −32 768. Promoted to int it becomes negative, making rowOffset negative → silent page corruption. Use dataPage.readShort(...) & 0xFFFF for unsigned semantics.

---

2. TimeSeriesSealedStore — file descriptor leak in constructor

engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesSealedStore.java

this.indexFile    = new RandomAccessFile(f, "rw");
this.indexChannel = indexFile.getChannel();
loadDirectory();   // or writeEmptyHeader() — both can throw

If loadDirectory() / writeEmptyHeader() throws, neither indexChannel nor indexFile is closed. The constructor should close both handles in a catch block (or use try-with-resources) on any failure path.

---

3. LineProtocolParser — no max-length guard on measurement name / tag fields

engine/src/main/java/com/arcadedb/engine/timeseries/LineProtocolParser.java

Measurement names, tag keys, and tag values are accumulated character-by-character with no upper bound. A crafted line with a multi-megabyte field name causes unbounded heap growth. Add a MAX_FIELD_LENGTH limit (e.g. 1 KB) and throw on overflow.

---

4. LocalTimeSeriesType.fromJSON — hard version check will break all existing databases on first format upgrade

engine/src/main/java/com/arcadedb/schema/LocalTimeSeriesType.java

if (sealedFormatVersion != TimeSeriesSealedStore.CURRENT_VERSION)
    throw new IllegalStateException("Unsupported sealed store format version ...");

CURRENT_VERSION is currently 0. When version 1 ships, every v0 database will fail to open with no migration path. Prefer a range check (version <= CURRENT_VERSION) and add a migration hook, or document that format version 0 is explicitly pre-GA and breaking is intentional.

---

5. PromQLEvaluator — pattern cache cleared unsafely under concurrent load

engine/src/main/java/com/arcadedb/engine/timeseries/promql/PromQLEvaluator.java

if (patternCache.size() >= MAX_PATTERN_CACHE)
    patternCache.clear();
return patternCache.computeIfAbsent(regex, r -> Pattern.compile(r));

ConcurrentHashMap.size() is only an estimate. Multiple threads can all see size >= MAX and all call clear() simultaneously, then race to re-populate — making the cache useless under burst load and hammering Pattern.compile. Replace with a proper bounded LRU (e.g. a LinkedHashMap wrapped in Collections.synchronizedMap with removeEldestEntry).

---

6. PromQLEvaluator.labelKey — per-call allocation inside the hot aggregation loop

engine/src/main/java/com/arcadedb/engine/timeseries/promql/PromQLEvaluator.java

private String labelKey(final Map<String, String> labels) {
    final List<String> sorted = new ArrayList<>(labels.keySet());
    Collections.sort(sorted);
    final StringBuilder sb = new StringBuilder("{");
    ...
}

This allocates an ArrayList + sort + StringBuilder on every call, which is inside the inner result-processing loop. For large range queries this adds significant GC pressure. Consider keeping label maps pre-sorted from construction or memoizing the key per map identity.

---

MEDIUM severity

7. SQLFunctionTimeBucket — interval multiplication can overflow, producing divide-by-zero

final long value = Long.parseLong(interval.substring(0, unitStart));
return switch (unit) {
    case "w" -> value * 7 * 86_400_000L;
    ...
};

A very large value silently overflows to 0 or a small negative, causing (timestampMs / intervalMs) * intervalMs to divide by zero or return a nonsensical bucket. Use Math.multiplyExact or add a range guard.

---

8. PromQLParser.parseDuration — accumulated total can overflow long

The per-component overflow guard (current > Long.MAX_VALUE / unitMs) is correct, but totalMs += current * unitMs itself can overflow after multiple valid components are accumulated. Add Math.addExact or check for overflow after each addition.

---

9. SQLFunctionRate — unbounded sample accumulation in memory

engine/src/main/java/com/arcadedb/function/sql/time/SQLFunctionRate.java

private final List<long[]> samples = new ArrayList<>();

A one-year window at 1-second granularity accumulates ~31 M rows per group in the aggregation phase. This is unnecessary: rate only requires firstTs, firstVal, lastTs, lastVal, and a running prevValue for counter-reset detection.

---

10. TimeSeriesBucket.isInArray — O(n) linear scan in the hot column-scan loop

private static boolean isInArray(final int value, final int[] array) {
    for (final int v : array) if (v == value) return true;
    return false;
}

Called for every (row × column) during a projected scan. Pre-sort columnIndices and use Arrays.binarySearch, or use a boolean[] presence bitmap indexed by column number.

---

11. SQLFunctionTimeBucket / SQLFunctionRate — missing null / type guards

• params[0].toString() in SQLFunctionTimeBucket throws NPE if the interval column is null.
• ((Number) params[0]).doubleValue() in SQLFunctionRate throws ClassCastException for non-numeric input. Add explicit null and instanceof checks with user-facing error messages.

---

12. PromQLEvaluator — unchecked cast of agg.param() to NumberLiteral

final int k = agg.param() != null ? (int) ((NumberLiteral) agg.param()).value() : 1;

If agg.param() is non-null but not a NumberLiteral, this silently throws ClassCastException. Add instanceof guard.

---

13. TimeSeriesEngine.aggregate — null values silently become 0.0

engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesEngine.java

else
    value = 0.0;

For MIN this incorrectly returns 0 even if all real values are positive. Use Double.NaN (and propagate NaN in aggregation) to match the semantics used by PromQLEvaluator.extractValue.

---

14. DictionaryCodec.encode — returns full backing array, not the filled slice

encode returns buf.array(), which includes unwritten trailing bytes if the buffer was not exactly filled. The pattern used by Simple8bCodec (buf.flip(); byte[] r = new byte[buf.remaining()]; buf.get(r); return r;) is safer and should be adopted here.

---

15. TimeSeriesShard.compact — no closed-guard, races with TimeSeriesEngine.close()

There is no volatile closed flag checked at the start of compact(). The maintenance scheduler can start a compaction after shardExecutor.shutdown() but before shard.close(), leaving the engine in a partially closed state. Add a guard to skip compaction once the engine is closing.

---

16. ContinuousAggregateImpl — monitoring metrics not atomically updated

refreshCount, refreshTotalTimeMs, refreshMinTimeMs, refreshMaxTimeMs are updated via separate atomic operations. A concurrent reader can observe refreshCount incremented but refreshTotalTimeMs not yet updated, producing a transiently incorrect average. Use a single lock or a version stamp if consistent snapshots matter.

---

LOW severity

• DeltaOfDeltaCodec.BitWriter: bitPos + additionalBits can overflow int for very large block sizes, bypassing the capacity check. Use long arithmetic for the sum before converting back to int.
• PromQLParser: Backtick-quoted label values are accepted (raw strings). Document or enforce that these are never used in downstream SQL construction, since backtick is ArcadeDB's SQL identifier-quote character.
• ContinuousAggregateImpl.fromJSON: loadedName can be null if the JSON key is absent; null is passed to the constructor without a non-null assertion.
• PromQLParser.parseDuration accumulation overflow: no test exercises a duration that is per-component valid but overflows long in total sum.

---

Test coverage gaps

Gap	Notes
GorillaXORCodec round-trip for Double.NaN / ±Infinity / -0.0	doubleToRawLongBits preserves NaN bit patterns; equality check in tests may fail silently
Phase 3 crash recovery in TimeSeriesShard.compact	Crash during writeTempCompactionFile followed by restart and stale-file cleanup path
DictionaryCodec.decode with crafted out-of-range dictionary indices	Verify the IOException path is triggered, not an ArrayIndexOutOfBoundsException
SQLFunctionRate with 1000+ counter resets in a long window	Verify accumulation does not OOM and result is numerically sane
PromQLParser.parseDuration accumulation overflow	Duration string that is per-component valid but overflows long when totalled

---

Overall this is a substantial and well-thought-out feature. The codec implementations (Gorilla XOR, delta-of-delta, Simple8b) are correctly structured, the SQL integration is clean, and the crash-recovery design is sound. Addressing the HIGH items — especially the readShort sign-extension bug and the constructor file-descriptor leak — before merge would make the module production-ready.

[claude]

test

[claude]

Code Review — Timeseries Model (PR #3511)

This is an impressive feature addition — a full time-series engine with columnar compressed storage, PromQL support, continuous aggregates, SIMD acceleration, and Grafana/Prometheus integration. The test coverage is extensive (29+ test classes) and the architecture is well-thought-out.

Previous review rounds have already caught and confirmed fixes for many issues (string overflow guard, metrics race condition, crash recovery, recursion depth limit, computeIfAbsent for scheduler, sanitizeTypeName allowlist). Below I focus on items that appear still open or newly observed.

---

Critical — Still Open

1. Non-atomic file swap in TimeSeriesSealedStore (4 locations)

All four file-rewrite paths (truncateBefore, rewriteWithBlocks, truncateToBlockCount, upgradeFileToVersion1) use:

if (!oldFile.delete() || !tmpFile.renameTo(oldFile))
    throw new IOException("...");

A crash or ENOSPC between delete() and renameTo() leaves the sealed store permanently gone. Use:

Files.move(tmpFile.toPath(), oldFile.toPath(), ATOMIC_MOVE, REPLACE_EXISTING);

This is the last remaining critical item per the 5th review pass.

2. TimeSeriesSealedStore.compressColumn() default case silently drops data

The TimeSeriesShard.compressColumn() correctly throws IllegalStateException for unknown codecs, but the sealed store's own compressColumn() still has:

default -> new byte[0];

This is invoked from downsampleBlocks(). An unknown codec silently produces an empty column that decodes to zero values — undetectable without a CRC mismatch. Should throw IllegalStateException.

---

High — New Observations

3. Simple8bCodec silently corrupts negative LONG values

Simple8bCodec is the default codec for LONG columns per ColumnDefinition.defaultCodecFor(). The codec is documented as supporting non-negative integers only, but there is no guard in TimeSeriesShard.compressColumn(). When a negative long is encoded, every selector has fits = false (negative values fail the bit-width check), so bestSelector stays at 15. The encoding stores only the lower 60 bits of the negative value, silently producing a large positive number on decode — irrecoverable corruption for any LONG field that can go negative (temperature, financial delta, etc.).

Fix: validate in compressColumn() that values are non-negative when using Simple8b, or zigzag-encode before passing to the codec (with matching decode), or document the constraint prominently.

4. PromQLEvaluator.evaluateRange() infinite loop when stepMs == 0

for (long t = startMs; t <= endMs; t += stepMs)

If stepMs == 0, this is an infinite loop that will hang the HTTP handler thread. A guard at the top of evaluateRange() is needed:

if (stepMs <= 0)
    throw new IllegalArgumentException("stepMs must be positive");

5. Transaction not rolled back on failure in TimeSeriesTypeBuilder.create()

try {
    database.begin();
    type.initEngine();
    database.commit();
} catch (final Exception e) {
    throw new SchemaException("...", e);
}

If initEngine() throws, the transaction is never rolled back, leaving an open transaction. Add database.rollback() in the catch block.

---

Medium

6. TimeSeriesMaintenanceScheduler ignores policy changes via ALTER TIMESERIES TYPE

computeIfAbsent correctly prevents duplicate task registration (good fix from earlier rounds), but it also means that if a retention or downsampling policy is changed via ALTER TIMESERIES TYPE and schedule() is called again, the old task with the old policy keeps running indefinitely. The method should cancel any existing task before re-registering.

7. Flat-mode pre-allocation OOM risk in MultiColumnAggregationResult

// pre-allocates double[maxBuckets][requestCount]

A query like SELECT avg(value) FROM sensor GROUP BY ts.timeBucket('1s') over a year ≈ 31.5M buckets × N columns. No upper bound is checked before this allocation. A configurable max-bucket guard (or automatic fallback to map mode) would prevent OOM crashes on wide-range queries.

8. buildFilteredQuery() doesn't handle ORDER BY without GROUP BY

In ContinuousAggregateRefresher, when a query has ORDER BY but no GROUP BY and no existing WHERE, the watermark clause is appended at the very end:

return query + " WHERE `" + tsColumn + "` >= " + watermark;

This produces invalid SQL like ... ORDER BY bucket WHERE ts >= .... Fix: also search for ORDER BY before falling back to end-of-string append.

9. Retention policy skips the mutable bucket

applyRetention(cutoffTimestamp) only calls shard.getSealedStore().truncateBefore(cutoffTimestamp). Data older than the retention window that has not yet been compacted will persist in the mutable bucket until the next compaction cycle and will be returned by queries. Consider applying the cutoff filter during mutable bucket reads as well.

10. PromQLEvaluator.patternCache thread-safety

computeIfAbsent() on a non-concurrent HashMap is documented as unsafe for concurrent access and can cause an infinite loop in some JDK versions. If PromQLEvaluator is ever accessed by multiple threads (e.g., concurrent HTTP requests), this will corrupt the map. Use ConcurrentHashMap. Additionally, the cache is unbounded — a client submitting many distinct regex patterns accumulates entries indefinitely. Consider a bounded LRU cache.

---

Minor / ArcadeDB Style

11. Integer[] boxing in TimeSeriesShard.sortIndices()

final Integer[] indices = new Integer[timestamps.length];

Allocates up to N Integer objects per compaction run, contrary to the "LLJ" (Low Level Java) design goal of minimal GC pressure. A primitive int[]-based argsort would be more consistent with the rest of the codebase.

12. tmpFile.delete() return value ignored in TimeSeriesSealedStore

if (tmpFile.exists())
    tmpFile.delete();

If deletion fails (e.g., locked on Windows), the store silently continues. Log a warning or throw to prevent operating in a potentially inconsistent state.

13. DatabaseAsyncAppendSamples write failures are not observable

Silent sample loss is particularly dangerous for time-series data. The async task wraps failures in an exception but the caller has no way to detect dropped writes. Consider exposing an error counter on the engine or providing a callback mechanism.

14. jdk.incubator.vector module documentation gap

The incubating module warning and the requirement for --add-modules jdk.incubator.vector in startup scripts should be prominently documented. Without this flag, the scalar fallback is active silently — users may assume SIMD is running when it is not. A startup log message from TimeSeriesVectorOpsProvider indicating which implementation is active (SIMD or scalar) would make this visible.

---

Strengths

• The crash-safe compaction protocol (flag + watermark + truncate-on-recovery) is well-designed and well-tested
• Block-level aggregation fast-path (skip decompression using pre-computed block stats) is a meaningful optimization
• The Gorilla XOR and Delta-of-Delta codec implementations correctly follow the referenced papers
• The BitReader 64-bit sliding window in DeltaOfDeltaCodec is a solid hot-path micro-optimization
• SIMD path with graceful scalar fallback via TimeSeriesVectorOpsProvider is the right design
• 29+ test classes covering accuracy, retention, downsampling, SQL integration, crash recovery, PromQL, and codec round-trips — excellent coverage
• CRC32 per-block integrity checks provide a solid safety net

Overall: the non-atomic file swap (#1), the silent data drop in compressColumn (#2), and the Simple8b negative-value corruption (#3) are the items I'd treat as blockers. The rest can be addressed before the final merge or tracked as follow-up issues. Great work on this substantial feature.

[claude]

Addendum — Additional Findings from Engine Analysis

A few more issues found on closer inspection of the core engine files:

High

File handles closed before the atomic move completes (TimeSeriesSealedStore)

In both truncateBefore() and rewriteWithBlocks(), the existing file handles (indexChannel, indexFile) are closed before the Files.move() call. If the move throws (e.g., ENOSPC, cross-device), the original file handles are already closed, leaving the store with no valid file and no way to recover. The handles should only be closed after a successful move.

TimeSeriesShard constructor: resource leak if sealedStore construction fails

this.mutableBucket = new TimeSeriesBucket(...);   // succeeds, file opened
this.sealedStore   = new TimeSeriesSealedStore(...); // throws -> mutableBucket never closed

If TimeSeriesSealedStore construction throws IOException, the already-constructed mutableBucket is orphaned with its file handle open. Wrap in try-catch and close mutableBucket on failure.

Medium

Unsynchronized access to blockDirectory in getter methods (TimeSeriesSealedStore)

Some getter methods access the block directory without holding directoryLock.readLock(). Concurrent writes (appending blocks) can produce a torn read. Getters should be consistent with the locking pattern used in write paths.

Read lock held during transaction rollback in appendSamples() (TimeSeriesShard)

The shard read lock is acquired outside the transaction boundary and released in a finally block that spans any rollback. If the rollback is slow (I/O contention), concurrent readers are blocked for longer than necessary. Consider releasing the lock before the rollback, or at minimum document the intended ordering.

---

These supplement the previous review comment. The file-handle ordering bug is worth fixing alongside the atomic-move change already flagged.

[claude]

Addendum 2 — Schema and SQL Integration Findings

A few more issues from the schema and SQL executor layer:

High

NPE in FetchFromTimeSeriesStep and AggregateFromTimeSeriesStep

Both execution steps call tsType.getEngine() and immediately invoke a method on the result without a null check. If initEngine() was never called or failed silently, the next query against that type throws NullPointerException rather than a meaningful error:

final TimeSeriesEngine engine = tsType.getEngine();   // can be null
resultIterator = engine.iterateQuery(...);             // NPE

Fix: check for null and throw a descriptive CommandExecutionException.

Watermark advancement not atomic with data commit (ContinuousAggregateRefresher)

The continuous aggregate refresher commits the new aggregated rows in a database transaction, then calls schema.saveConfiguration() to advance the watermark — but the watermark persistence is outside the transaction:

if (ca.getWatermarkTs() > watermark) {
  final LocalSchema schema = (LocalSchema) database.getSchema();
  schema.saveConfiguration();  // called AFTER transaction commit
}

If saveConfiguration() fails (I/O error, shutdown), the data is committed to the backing type but the watermark is not advanced. On the next refresh, the watermark-filtered query re-aggregates the same window, producing duplicate rows in the materialized view. The watermark should be persisted inside the same transaction as the data, or the backing type should be truncated and rebuilt if a mismatch is detected at startup.

Crash recovery gap for continuous aggregates (LocalSchema)

On startup, if a continuous aggregate status is BUILDING (interrupted refresh), the status is correctly reset to STALE. However, the watermark is not reset, so the next refresh only aggregates from the last watermark forward — missing any partially-written data from the interrupted run. Adding ca.setWatermarkTs(prevWatermark) (or rolling it back to the last-known-good checkpoint) before setting STALE would avoid silent data gaps.

Medium

findWhereIndex() does not handle SQL comments (ContinuousAggregateRefresher)

The character-by-character parser skips subquery depth but does not handle -- or /* */ comments. A continuous aggregate query containing an inline comment that includes the word WHERE (e.g., -- filter WHERE clause not needed) could match the comment content and inject the watermark condition at the wrong position, producing malformed SQL.

SAFE_COLUMN_NAME regex allows dot character

The pattern [A-Za-z0-9_.\\-]+ used to validate backingTypeName and bucketColumn permits . (dot), which could allow compound/qualified identifiers. Since these names are backtick-quoted in the generated SQL, this is low-severity, but tightening to [A-Za-z0-9_]+ would be safer and consistent with other validation in the codebase.

---

These are independent of the items in the previous two review comments.

[claude]

PR Review: Timeseries Model

This is an impressive and well-architected addition. The two-layer design (mutable ACID bucket + immutable columnar sealed store), crash-safe compaction protocol, and push-down aggregation all show careful engineering. Below is a detailed review.

---

Bugs / Correctness Issues

1. DeltaOfDeltaCodec: boundary off-by-one in 9-bit range check

DeltaOfDeltaCodec.java line 73:

} else if (dod >= -255 && dod <= 255) {
    writer.writeBits(0b110, 3);
    writer.writeBits(zigZagEncode(dod), 9);

The Javadoc says the range is [-256, 255], and indeed zigZagEncode(-256) = 511 = 2^9 - 1 fits exactly in 9 bits. The guard should be dod >= -256. As written, dod == -256 falls through to the 12-bit bucket using 4 extra bits unnecessarily. Not a correctness bug, but an encoding inefficiency that also contradicts the Javadoc contract.

2. Potential OOM from malformed / corrupted block data

Simple8bCodec.decode() lines 142-144:

if (totalCount < 0)
    throw new IOException("Simple8bCodec: negative count..." );
final long[] result = new long[totalCount];

A corrupted or maliciously crafted block with totalCount = Integer.MAX_VALUE would attempt to allocate an 8 GB array. The same issue exists in DictionaryCodec.decode() (the count read from offset 0). Both decoders should validate against MAX_BLOCK_SIZE before allocating.

3. aggregateMultiBlocks slow-path ignores pre-computed range bounds

TimeSeriesSealedStore.java lines 694-708 (no-bucket-interval path):

for (int i = 0; i < tsCount; i++) {
    final long ts = timestamps[i];
    if (ts < fromTs || ts > toTs) continue;

rangeStart/rangeEnd are computed via binary search just above (lines 641-642) for the bucketed path, but the non-bucketed path re-scans all tsCount timestamps with a per-element guard instead of using [rangeStart, rangeEnd). For a narrow time range in a dense block, this is measurably slower.

---

Performance / Memory

4. scanRange() scans all blocks (no binary search)

TimeSeriesSealedStore.scanRange() iterates blockDirectory linearly, while the companion iterateRange() correctly uses binary search to skip to the first overlapping block. The same optimization should apply to scanRange().

5. TimeSeriesEngine.query() materializes all data

TimeSeriesEngine.java line 121 collects all shard results into an unbounded ArrayList. On a dense series with millions of samples, this is an OOM risk. The lazy iterateQuery() path exists and is used by aggregate(); consider deprecating or bounding query().

6. adjustChunkForDictionaryLimit allocates a new HashSet per chunk

TimeSeriesShard.java line 750 allocates a HashSet<Object> for every chunk in buildCompressedBlocks(). For high-frequency compaction of high-cardinality columns this creates GC pressure. Reusing with clear() across chunks would halve the allocation rate here.

---

Concurrency / Locking

7. Maintenance scheduler thundering herd

TimeSeriesMaintenanceScheduler.java line 108: all registered types share the same fixed initial delay (5 000 ms). With many TimeSeries types, every maintenance task fires simultaneously at startup. A small type-index-based jitter (e.g., shardIndex * 1_000 ms) would spread the load.

8. All maintenance threads share the same name

TimeSeriesMaintenanceScheduler.java line 54: all threads are named "ArcadeDB-TS-Maintenance". A counter suffix ("ArcadeDB-TS-Maintenance-" + threadCounter.getAndIncrement()) would make heap dumps and thread lists much easier to read.

---

Format Versioning / Upgrade Path

9. Hard IllegalStateException on format version mismatch blocks database open

LocalTimeSeriesType.fromJSON() lines 182-190 throws IllegalStateException if either sealedFormatVersion or mutableFormatVersion doesn't equal the current constant. With both constants currently at 0, any future format change would prevent existing databases from opening without a migration path. Consider a migration/upgrade mechanism (or at least downgrade to a recoverable DatabaseException with a clear message pointing to a migration guide) before the format version ever needs to change.

---

Security

10. PromQL ReDoS guard: good start, but incomplete

PromQLEvaluator.java lines 80-83: the REDOS_CHECK pattern catches basic nested quantifiers and alternation-with-quantifier patterns, and the combination of MAX_REGEX_LENGTH = 1024, the LRU pattern cache, and the guard provides reasonable defense-in-depth. Worth noting that the guard does not detect all catastrophic patterns (e.g., (a+a+)+), so a compile-time timeout (via a custom ThreadLocal matcher with a timeout) would be a stronger guarantee for production deployments that allow user-supplied PromQL.

---

Minor / Nits

• TimeSeriesSealedStore.CURRENT_VERSION = 0: Consistent with what's persisted; but calling it CURRENT_VERSION when it is also the only-ever version means there is no migration tested. A comment noting "bump this when the block format changes and add a loadDirectory migration path" would help future maintainers.
• Double-rollback in appendSamples() (TimeSeriesShard.java lines 163-165): the two consecutive rollback calls are well-commented but are a surprising pattern. A helper such as rollbackAll() with a clear explanation would make the intent more obvious to future readers.
• HA implication of sealed stores bypassing replication: the Javadoc on TimeSeriesSealedStore explains this clearly. Worth also calling out in the user-facing documentation that immediately after failover a replica may return partial query results until its next compaction cycle completes.

---

What's Well Done

• The crash-safe five-phase compaction protocol in TimeSeriesShard.compact() is well-engineered: the compaction-in-progress flag + watermark committed in Phase 0 means any crash between Phase 0 and Phase 4 is safely recoverable at the next open.
• The lock ordering in aggregateMulti() (all compaction read locks acquired in shard order on the calling thread before submitting futures) correctly avoids the sealed-vs-mutable data loss window.
• DeltaOfDeltaCodec.BitReader's sliding 64-bit register avoids per-bit array access.
• The REDOS guard + LRU pattern cache in PromQLEvaluator is a good security addition for a user-facing query interface.
• The primitive merge-sort in TimeSeriesShard.sortIndices() avoids Integer[] boxing—good alignment with the project's GC-pressure mandate.
• LineProtocolParser.sanitizeForLog() correctly strips control characters before logging user-supplied measurement names.

[claude]

Code Review: Timeseries Model (#3511)

This is a substantial and well-designed contribution. The architecture is sound: LSM-tree-style lock-free compaction, columnar sealed storage with block-level stats for aggregation push-down, Gorilla/Delta-of-Delta/Simple-8b codecs, and InfluxDB line protocol support. The test coverage is extensive. Below are the issues I found, from most to least critical.

---

Concurrency: Temp-file path collision (TimeSeriesShard + TimeSeriesSealedStore)

Phase 3 of compact() calls sealedStore.writeTempCompactionFile() without any lock on directoryLock. Meanwhile, truncateBefore() and downsampleBlocks() each acquire directoryLock.writeLock() and also write to the same .ts.sealed.tmp path.

compactionMutex in TimeSeriesShard only prevents concurrent compact() calls — it does not prevent an explicit compact() (e.g. via SQL) from running its Phase 3 concurrently with the maintenance scheduler running applyRetention(). If that happens, both code paths write to the same temp file, corrupting each other's output.

The maintenance scheduler runs compact→retention→downsampling sequentially within a single task, so within one scheduled tick this cannot race. But a user-triggered COMPACT TIMESERIES TYPE <name> happening during the window between a scheduled compact completing and retention starting IS a realistic race.

Suggested fix: either extend compactionMutex to also gate truncateBefore()/downsampleBlocks(), or use a per-shard dedicated mutex for all operations that touch the temp file.

---

Correctness: iterateQuery() min-heap assumes sorted mutable output

TimeSeriesEngine.iterateQuery() uses a PriorityQueue keyed on each shard iterator's next timestamp. This requires each shard iterator to produce monotonically increasing timestamps.

TimeSeriesShard.iterateRange() returns sealed data (sorted by compaction) followed by mutable data from mutableBucket.scanRange(). The mutable scan returns rows in page-insertion order, which is not guaranteed to be timestamp-sorted for out-of-order writes.

Result: if samples are ingested out of order (a valid scenario for distributed producers), iterateQuery() produces incorrect merge-sorted output for data that has not yet been compacted.

Suggested fix: either (a) document that the mutable bucket requires in-order writes, (b) sort mutable results in iterateRange() before returning them, or (c) add a secondary heap-merge for sealed vs mutable within a shard.

---

Code quality: Indentation inside aggregateMultiBlocks()

In TimeSeriesSealedStore.java (around line 535), the for loop body is NOT indented inside the try block — it sits at the same level as try {. The closing brace pair makes this especially hard to parse:

directoryLock.readLock().lock();
try {
for (final BlockEntry entry : blockDirectory) {   // should be indented one level
    ...
}
} finally {
    directoryLock.readLock().unlock();
}

Since the lock scope is the most safety-critical aspect of this method, the indentation should clearly show the lock is held for the entire loop.

---

Performance: String bytes read one-by-one in TimeSeriesBucket.readColumnValue()

case STRING -> {
    final int len = page.readShort(offset) & 0xFFFF;
    final byte[] bytes = new byte[len];
    for (int i = 0; i < len; i++)
        bytes[i] = (byte) page.readByte(offset + 2 + i);  // one call per byte
    yield new String(bytes, StandardCharsets.UTF_8);
}

Reading up to 256 bytes one at a time adds unnecessary overhead on hot read paths. A bulk page.readByteArray(offset + 2, bytes, 0, len) (or equivalent) would be more consistent with the engine's I/O patterns and notably faster.

---

Performance: Per-sample header page write in appendSamples()

TimeSeriesBucket.appendSamples() calls updateHeaderStats(tx, timestamps[i]) inside the per-sample loop. Each call invokes tx.getPageToModify(..., page 0, ...). For a batch of N samples this is N individual calls to get the header mutable page.

Min/max timestamps and sample count can be computed across the whole batch first, then a single post-loop header update would be both correct and cheaper.

---

Performance: Fixed row size for STRING wastes up to 258 bytes per column per row

calculateRowSize() allocates 2 + MAX_STRING_BYTES (258 bytes) per STRING column regardless of actual content. For schemas where TAG values are short (e.g., "us-east-1" = 9 bytes), 249 bytes per row per column are wasted. This significantly reduces rows-per-page for string-heavy schemas and increases mutable bucket file size.

This is a conscious design choice (fixed row stride = O(1) offset arithmetic), but it should be documented in the class Javadoc since it affects capacity planning.

---

DeltaOfDeltaCodec Javadoc range typo

The Javadoc says dod in [-256, 255] but the code checks dod >= -255 && dod <= 255. The actual range encoded in 9-bit ZigZag is [-255, 255]. Minor but misleading to anyone extending the codec.

---

ConcurrentModificationException in appendSamples silently rolls back outer TX

TimeSeriesShard.appendSamples() rolls back the nested TX on ConcurrentModificationException, then unconditionally rolls back again if any TX is still active. The comment explains this is intentional to restore the caller to a clean state, but this silently rolls back the caller's enclosing transaction without explicit notification. Callers who have done work in an outer TX before calling appendSamples will lose that work.

This should either be documented on the public API (TimeSeriesEngine.appendSamples), or the second rollback should be conditional on the engine being able to confirm the outer TX has no prior work.

---

Minor observations

• isInArray() in TimeSeriesBucket.readRow() is O(k) per column where k = requested column count. A BitSet would be faster for wide schemas.

• HA note should surface in server docs: The fact that .ts.sealed files bypass page replication (each HA node compacts independently) is critical operational knowledge documented in TimeSeriesSealedStore Javadoc, but it should also appear in server HA documentation so operators are not surprised by post-failover queries that return only mutable-bucket data until the next compaction cycle.

• Version 0 with no migration path: Both CURRENT_VERSION = 0 fields validate on open and throw IllegalStateException on mismatch. Future format changes will need a migration path — a placeholder comment for where that logic belongs would help.

---

Overall this is a well-engineered feature with excellent inline documentation and strong test coverage. The concurrency issue around the temp file and the heap-merge ordering assumption on unsorted mutable data are the two items I would want resolved before merge.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -9.98%	✅ 79.41%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (cd9a218)	93306	69482	74.47%
Head commit (bdf2b24)	130669 (+37363)	84269 (+14787)	64.49% (-9.98%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3511)	6859	5447	79.41%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[claude]

Code Review — PR #3511: Timeseries model

This is an impressive and substantial addition. The architecture is well thought out: LSM-style lock-free compaction, Gorilla/delta-of-delta codecs, SIMD-accelerated aggregation, block-level fast-path stats, PromQL integration, and crash-safe recovery. The inline documentation is excellent and the test coverage is broad. Below are issues worth addressing before merge.

---

Bug: Double rollback may silently discard a caller's outer transaction

File: engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesShard.java:166–177

} catch (final ConcurrentModificationException cme) {
    if (database.isTransactionActive())
        database.rollback();
    // Also roll back the enclosing TX ...
    if (database.isTransactionActive())
        database.rollback();   // ← rolls back caller's outer TX
    throw cme;
}

The second rollback() terminates the caller's outer transaction. The Javadoc on appendSamples() states "the caller's outer transaction remains unaffected", but this handler violates that contract: if the caller had dirty pages in an outer transaction, they are silently discarded. Even if the intent is to leave a clean state for a retry loop, the method has no way to know whether the caller cares about that outer transaction. Consider omitting the second rollback and letting the caller decide whether to roll back/retry their own transaction.

---

Potential metadata inconsistency after appendBlock()

File: engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesSealedStore.java:178–260

appendBlock() appends compressed data to the file and sets headerDirty = true, but does not flush the header. The header (which stores the block count and global min/max timestamps) is only persisted by flushHeader() / close(). A crash between an appendBlock() call and the next close() will leave the header showing a stale block count that is smaller than the actual file content. On restart, loadDirectory() would need to scan the file to repair this — it's worth confirming that loadDirectory() handles this case (scans past the header-reported count) or, alternatively, flushing the header atomically after each append.

---

iterateRange() is misleadingly named — it eagerly materialises all results

Files: TimeSeriesSealedStore.java:345–410, TimeSeriesShard.java:220–280

Both methods return an Iterator<Object[]> but internally collect all matching rows into an ArrayList before returning results.iterator(). For large time ranges with millions of rows this can cause OOM or very large GC pauses, contrary to what the iterate* naming implies. The comment in TimeSeriesShard explains the motivation (holds the lock only while reading, then releases it), which is sound, but the name should reflect this (e.g. scanRangeAsIterator) or the Javadoc should prominently note that results are fully materialised before the iterator is returned. Users querying unbounded time ranges via SQL will be surprised.

---

Inconsistent indentation in aggregateMultiBlocks()

File: engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesSealedStore.java:534–535

    directoryLock.readLock().lock();
    try {
    for (final BlockEntry entry : blockDirectory) {   // ← missing indent

The for loop body is not indented inside the try block. The closing } finally { is also misaligned. This makes it hard to see that the entire loop runs under the read lock.

---

jdk.incubator.vector — incubating API concerns

File: engine/src/main/java/com/arcadedb/engine/timeseries/simd/SimdTimeSeriesVectorOps.java

jdk.incubator.vector is still in the incubator module in JDK 21 and requires --add-modules jdk.incubator.vector on both the compile and runtime command lines. Please verify that:

1. The engine/pom.xml includes the correct <compilerArg> and <jvmArg> entries.
2. The server startup scripts (server.sh / server.bat) pass --add-modules jdk.incubator.vector to the JVM.
3. The TimeSeriesVectorOpsProvider gracefully degrades to ScalarTimeSeriesVectorOps on JVMs where the module is absent (it appears to already try/catch for this — just confirming this is tested).

---

HA / replication gap for sealed store data

File: engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesSealedStore.java:73–84 (Javadoc)

The Javadoc correctly documents that .ts.sealed files bypass page-level replication. However, the consequence deserves prominence in the user-facing documentation (docs/timeseries.md): immediately after a leader failover, a follower that hasn't compacted yet will serve queries from the mutable bucket only, and historical data that was in the sealed store of the old leader will be invisible until the new leader runs its own compaction cycle. Operators should know to configure an aggressive compaction interval in HA deployments.

---

TagFilter.eq() throws NPE on null value

File: engine/src/main/java/com/arcadedb/engine/timeseries/TagFilter.java:46–50

public static TagFilter eq(final int nonTsColumnIndex, final Object value) {
    ...
    conditions.add(new Condition(nonTsColumnIndex, Set.of(value)));  // NPE if value == null

Set.of(null) throws NullPointerException. Tag values may legitimately be absent/null (e.g. a row with no tag value). Either validate and throw a clear IllegalArgumentException, or use a nullable-safe alternative such as Collections.singleton(value).

---

Maintenance thread names are all identical

File: engine/src/main/java/com/arcadedb/engine/timeseries/TimeSeriesMaintenanceScheduler.java:54–58

All threads in the pool are named ArcadeDB-TS-Maintenance. With MAX_THREADS = 4 and potentially many TimeSeries types registered, thread dumps will have four indistinguishable threads. Adding an AtomicInteger counter (or including the type name when the task runs) would aid production debugging.

---

Minor: LocalTimeSeriesType.fromJSON() strict version check

File: engine/src/main/java/com/arcadedb/schema/LocalTimeSeriesType.java:182–190

The equality check (!= CURRENT_VERSION) for format versions is appropriate for preventing data corruption, but when CURRENT_VERSION is incremented in the future, any schema file that lacks the version field will default to 0 and silently pass (since the default in json.getInt("sealedFormatVersion", 0) happens to equal version 0). This will become a silent false positive once either version exceeds 0. Storing the version field unconditionally in toJSON() (which it already does) is fine; consider also adding a migration path or upgrade note in the schema loading documentation.

---

Summary

The core architecture is sound and performant. The crash-recovery via compaction watermark, the LSM-style lock-free compaction phases, and the block-level aggregation fast path are all well-implemented. Addressing the double-rollback bug and the appendBlock() header durability question should be prioritised before merge.

[claude]

Code Review: TimeSeries Module

This is a substantial and well-architected contribution. The LSM-style sharded storage, multi-tier downsampling, crash-safe compaction, Gorilla/Delta-of-Delta codecs, and SIMD vectorization are all thoughtful choices. The test coverage breadth is impressive. Below are issues worth addressing before merging.

---

Critical / High Priority

1. SIMD module runtime crash risk

SimdTimeSeriesVectorOps requires --add-modules jdk.incubator.vector. The compiler flag is added in engine/pom.xml, but the flag also needs to be present at runtime (JVM startup). If the server or any downstream process that embeds ArcadeDB launches without this flag, TimeSeriesVectorOpsProvider will fail with NoClassDefFoundError at class-loading time rather than falling back gracefully to ScalarTimeSeriesVectorOps.

Recommendation: Wrap the SIMD class instantiation in TimeSeriesVectorOpsProvider with a try { ... } catch (NoClassDefFoundError | UnsupportedOperationException e) block, and verify the fallback is exercised in the test suite.

2. SQL injection in ContinuousAggregateRefresher

The SAFE_COLUMN_NAME regex correctly validates column/type identifiers, but ca.getQuery() (the user-supplied base query) is embedded verbatim into a generated SQL string without any sanitisation. A user with DDL privileges who stores a malicious CREATE CONTINUOUS AGGREGATE query can execute arbitrary SQL during every background refresh cycle. The query should be treated as untrusted, or at minimum its injection surface should be documented and bounded.

3. extractTimeRange uses toString() for column name matching

In SelectExecutionPlanner, the method uses expr.toString().trim() to compare against the timestamp column name. Quoted identifiers (e.g. timestamp_col), expressions containing whitespace, or case differences will fail to match, and the planner silently falls back to a full scan with no warning. This is a correctness issue: queries that should use the time-range push-down won't, but users won't know why.

Recommendation: Use the AST node directly (e.g. BaseExpression / Identifier comparison) rather than a string comparison on toString().

4. Tag filter extraction is incorrect for OR predicates

extractTagFilter iterates all AndBlocks in flattenedWhereClause and AND-combines the extracted tag predicates across blocks. For a WHERE clause like timestamp BETWEEN x AND y OR tag = 'A', this produces a tag filter that is too restrictive (it treats OR branches as AND). Either restrict push-down to queries with a single AndBlock, or correctly handle the OR semantics (skip tag push-down when OR is present).

5. TagFilter: Set.of(value) throws NullPointerException for null tag values

If a tag value is legitimately null or missing in a sample, any call to TagFilter.eq() or TagFilter.in() that receives that null value will throw a NPE at Set.of(value). Time-series data often has sparse tags. Replace Set.of() with a null-safe collection, or explicitly reject null values with a descriptive exception at query-parse time.

---

Medium Priority

6. DeltaOfDeltaCodec.BitReader buffer overread near end of array

refill() loads 8 bytes at a time: data[bytePos] … data[bytePos+7]. When fewer than 8 bytes remain, the long-read goes past the array boundary and throws ArrayIndexOutOfBoundsException. The codec is presumably tested to work, so this might be guarded upstream by padding—but it should be made explicit and safe within the codec itself.

7. Shallow clone in DatabaseAsyncAppendSamples

The constructor clones columnValues shallowly (columnValues[i].clone()). If the Object values inside the rows are themselves mutable (e.g. byte[] for BLOB columns), a caller that reuses buffers after submission can silently corrupt in-flight async writes. Deep-copy, or document that callers must not mutate values after submission.

8. No explicit transaction in DatabaseAsyncAppendSamples.execute()

The execute method calls engine.getShard(shardIndex).appendSamples() without beginning or committing a transaction. ArcadeDB requires an active transaction for writes. Either TimeSeriesShard.appendSamples must internally manage its own transaction (in which case that fact should be prominently documented, since it differs from the standard API pattern), or the async task should wrap the call in database.begin() / database.commit().

9. DatabaseContext thread initialization in the maintenance scheduler

The maintenance scheduler submits tasks to a thread pool. If DatabaseContext.INSTANCE.init() is not called on those pool threads before the first use of any DatabaseContext-dependent code, NPEs or stale context reads can occur. Verify that the maintenance tasks properly initialize and clean up thread-local database context, and add a test that exercises maintenance on a non-trivial database.

---

Low Priority / Style

10. isCheckingDatabaseIntegrity() returns false in engine tests

Multiple test classes override isCheckingDatabaseIntegrity() to return false. This suppresses integrity checks that could catch storage-layer bugs introduced by the new engine. At minimum the sealed-store compaction tests (BucketAlignedCompactionTest, TimeSeriesCrashRecoveryTest) should run with integrity checks enabled.

11. Missing test for applyRetention and applyDownsampling

TimeSeriesEngineTest has good happy-path coverage but no tests for the data lifecycle management paths: applyRetention(cutoffTs), applyDownsampling(tiers, nowMs), and compactAll() with a partially-written bucket. These are the paths most likely to corrupt data on crash.

12. toEpochMs LocalDate fallback is timezone-ambiguous

In SelectExecutionPlanner.toEpochMs(), when Instant.parse() fails the code falls back to LocalDate.parse().atStartOfDay(ZoneOffset.UTC). A user who writes WHERE timestamp = '2025-01-01' will get midnight UTC, which may not match their local timezone. This should either be documented clearly or accept a timezone parameter.

13. extractTimeRange and push-down failures are silent

When time-range extraction fails or push-down is not eligible, the planner falls back to a full scan with no log entry. For a performance-critical feature this makes it hard to diagnose why a query is slow. Consider a LogManager.instance().log(Level.FINE, ...) trace when push-down is skipped.

14. engine/pom.xml --add-modules should also be reflected in the server startup scripts

The incubator module flag added to the Maven compiler does not automatically propagate to server.sh / server.bat. If customers run the packaged server without adding -Djdk.incubator.vector or updating JAVA_OPTS, SIMD will silently be unavailable (if fallback is implemented) or crash (if not). The startup scripts and documentation should be updated.

---

Positive Observations

• The multi-phase lock-free compaction design in TimeSeriesShard is well thought out, with explicit crash-recovery markers and an atomic file swap for the sealed store.
• The codec implementations (Gorilla XOR, Delta-of-Delta, Simple-8b, Dictionary) follow published specifications with correct ZigZag encoding. The pre-allocated decode buffer variant avoids allocation in the hot path—a good performance choice.
• The block-level tag pruning (SKIP / FAST_PATH / SLOW_PATH) in TimeSeriesSealedStore.blockMatchesTagFilter is an elegant multi-level optimization.
• The InfluxDB line protocol parser correctly handles backslash escaping, log-injection prevention, and type suffixes per the InfluxDB spec.
• The assertThat-style assertions are consistently used across the test suite.
• ATTRIBUTIONS.md and NOTICE updates are present for the new dependency (Snappy).

Overall this is high-quality work. The critical issues above (particularly the SIMD crash risk, SQL injection surface, and tag-filter OR semantics) should be addressed before merge.

[claude]

Code Review — TimeSeries Module

This is a substantial and well-designed feature. The architecture is solid and the code demonstrates careful thinking about performance, crash safety, and concurrency. Below are observations ranging from bugs to minor nitpicks.

---

Potential Bugs

1. DeltaOfDeltaCodec — Javadoc/code range mismatch

The class Javadoc documents the 9-bit ZigZag bucket as dod in [-256, 255], but the implementation checks dod >= -255 && dod <= 255:

// DeltaOfDeltaCodec.java line 73
} else if (dod >= -255 && dod <= 255) {
    writer.writeBits(0b110, 3);
    writer.writeBits(zigZagEncode(dod), 9);

This is a 1-value discrepancy. The Gorilla paper uses asymmetric ranges, but the documented format says -256 while the code encodes starting at -255. Since this is a custom binary format, any future external decoder built from the docs will be subtly incompatible with the encoder. The code is internally consistent (encoder and decoder agree at -255), but the Javadoc should say [-255, 255].

---

2. loadDirectory() — globalMinTs/globalMaxTs not updated during block scanning

loadDirectory() reads globalMinTs/globalMaxTs from the header (line 1670–1671) but the subsequent file scan that rebuilds blockDirectory does not update these volatile fields. If blocks are recovered beyond what the header recorded (crash-recovery path), getGlobalMinTimestamp()/getGlobalMaxTimestamp() return stale header values.

In practice this causes only suboptimal flat-array sizing in TimeSeriesEngine.aggregateMulti() (falls back to map mode), not data loss. But the contract between getBlockCount() > 0 and a meaningful global range is broken. Consider updating globalMinTs/globalMaxTs from scanned blocks during loadDirectory().

---

Performance Concerns

3. PromQLEvaluator.evaluateRange() — O(steps × lookbackWindow) scans

For range queries, evaluate() is called once per step, and each call to evaluateVectorSelector executes engine.iterateQuery(queryStart, queryEnd, ...) over a full [evalTime - lookbackMs, evalTime] window. For a 1-hour range query with 60-second steps and a 5-minute lookback:

• 60 calls to iterateQuery
• Each scan covers 5 minutes of data
• The same data is re-scanned ~5× unnecessarily

A single pass over [startMs - lookbackMs, endMs] is sufficient for all steps. This is a known complexity issue in naive PromQL implementations, but it will be noticeable at scale. Consider materialising the full scan once and then slicing per step for vector selectors.

4. loadDirectory() — many small ByteBuffer.allocate() calls per block

For each block, the method allocates 5–8 separate small ByteBuffer objects (baseMetaBuf, numBuf, statsBuf, tagCountBuf, per-tag dcBuf/lenBuf/valBuf, crcBuf). For a sealed store with tens of thousands of blocks, this is significant GC pressure at startup. A single pre-allocated buffer re-positioned via absolute reads would eliminate these allocations.

---

Design / API Issues

5. iterateRange() on TimeSeriesSealedStore is not actually lazy

The method is named iterateRange and returns an Iterator, implying lazy evaluation. The Javadoc's "Note" section does document materialisation, but callers seeing only the method signature will be surprised. The existing scanRange() (which returns List<Object[]>) already exists for materialised results. Having both a scanRange and an iterateRange that both materialise makes the API confusing — consider consolidating or making the lazy contract explicit in the type signature (e.g., by returning List instead of Iterator in iterateRange, or removing scanRange in favour of it).

6. TimeSeriesEngine.aggregate() vs aggregateMulti() — inconsistent column index semantics

The Javadoc on aggregate() (line 169) explicitly warns that its columnIndex parameter is "0-based among non-timestamp columns" while MultiColumnAggregationRequest.columnIndex() uses the full schema index. This inconsistency requires callers to know which method they're calling and apply a different indexing convention. The two methods are not interchangeable, but they appear to serve the same use case. Consider unifying to one convention and documenting it clearly on both.

7. ContinuousAggregateRefresher.buildFilteredQuery() — SQL string manipulation of stored queries

The query stored in a continuous aggregate is user-provided and then modified by string manipulation to inject a watermark filter. The findWhereIndex() parser handles comments, string literals, and nested parentheses correctly for standard SQL, but it does not handle CTEs (WITH ... AS (...)), which the existing code comment acknowledges. A query like:

WITH cte AS (SELECT ... FROM src WHERE ts > 0) SELECT ... FROM cte GROUP BY ...

would have findWhereIndex find the inner WHERE, inserting the watermark filter inside the CTE instead of at the top level, returning incorrect results silently. Consider adding a check that rejects queries containing WITH at the top level rather than silently applying an incorrect filter.

---

Minor Notes

8. Compaction errors silently swallowed in TimeSeriesMaintenanceScheduler

The maintenance task catches Throwable and logs at WARNING. If compaction repeatedly fails (e.g., due to a file permission issue), the mutable bucket grows unboundedly with no mechanism to alert the operator beyond log messages. Consider exposing a failure counter via the existing metrics/monitoring infrastructure.

9. sanitizeTypeName() logs on first call per name but pattern is shared

VALID_TYPE_NAME_PATTERN uses matches() which is anchored, so names cannot bypass validation. However, sanitizeTypeName is public static and uses LogManager.instance() which ties the utility to the ArcadeDB runtime. If this method is later called in tests or CLI utilities without a running instance, it will fail. Consider making the logger optional or the method package-private.

10. TimeSeriesBucket.MAX_STRING_BYTES = 256 is undocumented as a schema constraint

This constant limits TAG column values to 256 UTF-8 bytes. This constraint is not surfaced at type creation time (the CREATE TIMESERIES TYPE statement doesn't validate it), so a user who inserts a longer tag value will get silent truncation or an error at write time without a clear diagnostic. Consider validating at DDL time and documenting the limit in the schema error message.

---

What's Done Well

• The 5-phase compaction design with crash-recovery flag + watermark is elegant and correct. The compactionInProgress + truncateToBlockCount(watermark) restart path properly handles all crash windows.
• Block-level statistics (min/max/sum) enabling aggregation push-down without decompression is a major optimisation.
• The BitReader sliding 64-bit window significantly reduces array accesses vs. the naive Gorilla implementation.
• ReDoS protection with REDOS_CHECK pattern in PromQLEvaluator is good defensive coding.
• SQL injection prevention in ContinuousAggregateRefresher via SAFE_COLUMN_NAME validation is correct (uses matches() which anchors the pattern).
• WeakReference<Database> and WeakReference<LocalTimeSeriesType> in the maintenance scheduler correctly prevent memory leaks when a database is closed.
• Comprehensive test suite covering accuracy, crash recovery, concurrency, downsampling, and dictionary overflow.

[claude]

Code Review — TimeSeries Module

This is a large, ambitious feature and the overall quality is high. The compression codec choices are well-founded (Gorilla/DoD/Simple8b/Dictionary), the 5-phase lock-free compaction with crash recovery is carefully designed, and the test coverage is excellent (20+ test classes covering crash recovery, concurrent compaction, downsampling, format versioning, gap analysis, and more). Below are observations ranging from bugs to minor nits.

---

🔴 Bugs

1. WHERE clause operator-precedence bug in ContinuousAggregateRefresher.buildFilteredQuery (line 142)

When the source query already has a WHERE clause, the watermark filter is injected as:

WHERE `ts` >= 1234 AND <existing condition>

If <existing condition> contains OR, SQL precedence (AND binds tighter than OR) changes the semantics:

-- Original:   WHERE a = 1 OR b = 2
-- Result:     WHERE ts >= 1234 AND a = 1 OR b = 2
-- Equivalent: WHERE (ts >= 1234 AND a = 1) OR b = 2

Rows matching only b = 2 bypass the watermark filter entirely. The existing condition should be wrapped in parentheses:

return before + " `" + tsColumn + "` >= " + watermark + " AND (" + after.stripLeading() + ")";

validateQueryStructure() rejects CTEs and subqueries but not complex WHERE clauses with OR, so this is reachable in practice.

2. SelectExecutionPlanner.toEpochMs silently returns Long.MIN_VALUE on unknown types (line 1775)

Unlike SaveElementStep.toEpochMs (throws CommandExecutionException) and ContinuousAggregateRefresher.toEpochMs (returns 0), the planner variant returns Long.MIN_VALUE:

return Long.MIN_VALUE;  // silent fallback

A WHERE condition like WHERE ts = someUnsupportedType would result in a query over a nonsensical time range instead of failing with a useful error. This should throw like the other variants.

---

🟡 Design / Architecture

3. toEpochMs duplicated in four places with divergent behavior

The same timestamp-conversion logic exists in:

• SaveElementStep.toEpochMs(Object, ZoneId) — most complete, handles LocalDateTime/LocalDate/String/Instant
• SQLFunctionTimeBucket.toEpochMs(Object) — no LocalDate, uses ZoneOffset.UTC for LocalDateTime
• SelectExecutionPlanner.toEpochMs(Object) — returns Long.MIN_VALUE for unknown types
• ContinuousAggregateRefresher.toEpochMs(Object) — handles only Date/Long/Number, returns 0 otherwise

These should be consolidated into a single utility method (e.g., TimeSeriesUtils.toEpochMs). The inconsistencies in fallback behavior (throw vs. 0 vs. Long.MIN_VALUE) can cause subtle, hard-to-debug query behaviour differences.

4. parseInterval duplicated between SQLFunctionTimeBucket (line 63) and ContinuousAggregateBuilder (line 197)

The two implementations are byte-for-byte identical. SelectExecutionPlanner already correctly delegates to SQLFunctionTimeBucket.parseInterval(), but ContinuousAggregateBuilder maintains its own copy. Consolidate by having ContinuousAggregateBuilder call SQLFunctionTimeBucket.parseInterval().

5. TimeSeriesSealedStore bypasses HA replication

The sealed store intentionally uses RandomAccessFile/FileChannel outside ArcadeDB's page-management infrastructure. As a consequence, sealed (compacted) time-series data is not replicated in HA/cluster mode. This is a significant architectural gap for production deployments and deserves a prominent callout in the docs and Javadoc — users relying on HA will have silent data divergence on replicas after compaction runs.

---

🟡 Correctness / Validation

6. DownsamplingTier missing cross-field validation (DownsamplingTier.java)

The compact constructor validates afterMs > 0 and granularityMs > 0 but not granularityMs <= afterMs. A tier like afterMs=1d, granularityMs=1w (downsample to weekly granularity on data older than 1 day) is logically nonsensical and should be rejected. The SQL parser (AlterTimeSeriesTypeStatement) and TimeSeriesTypeBuilder also do not validate this.

7. SAFE_COLUMN_NAME allows digit-leading identifiers (ContinuousAggregateRefresher.java:119)

private static final Pattern SAFE_COLUMN_NAME = Pattern.compile("[A-Za-z0-9_]+");

This permits names like 1column. These are technically injectable with backtick quoting (1column is valid SQL) so the injection concern is addressed, but it diverges from ArcadeDB's own identifier rules. Add (?!\d) or change to [A-Za-z_][A-Za-z0-9_]* for consistency.

---

🟡 Code Quality

8. TimeSeriesSealedStore.java — 2013 lines is too large

This single file contains file I/O, block directory management, aggregation logic, decompression, block-level stats, CRC validation, downsampling, compaction temp-file writing, and retention. Consider extracting at minimum BlockDirectory and the aggregation engine into separate classes to improve navigability and testability.

9. TimeSeriesMaintenanceScheduler.MAX_THREADS = 4 hardcoded

With many TimeSeries types or high downsampling workloads, 4 threads may be insufficient or wasteful. This should be driven by a GlobalConfiguration entry, following the pattern used elsewhere in ArcadeDB.

10. jdk.incubator.vector requires explicit JVM argument

SimdTimeSeriesVectorOps uses the Java Vector API (incubator module). The --add-modules jdk.incubator.vector argument is added to the engine compiler configuration, but ensure it is also present in:

• Server startup scripts (server.sh / server.bat)
• Test runner JVM args (Surefire/Failsafe config)
• Any Docker image JAVA_OPTS

TimeSeriesVectorOpsProvider gracefully falls back to scalar ops if the incubator module is absent, which is good, but without the JVM flag the SIMD path will never be used even on capable hardware.

---

🟢 Positive Observations

• Crash recovery tests (TimeSeriesCrashRecoveryTest) are thorough — injecting compaction flags manually and verifying cleanup is exactly the right approach.
• ReDoS protection in PromQLEvaluator (pattern complexity check + length cap + Collections.synchronizedMap LRU cache) is solid.
• Log-injection prevention in LineProtocolParser.sanitizeForLog() is a good practice.
• Defensive copying in DatabaseAsyncAppendSamples (clone of both timestamps and each columnValues[i]) is correct for async handoff.
• Atomic temp-file rename pattern in TimeSeriesSealedStore prevents partial-write corruption across all mutation paths.
• The PromQLEvaluator correctly returns an empty InstantVector rather than throwing when a metric name does not map to a known TimeSeries type.
• AggregationMetrics thread-safety contract (per-shard instances + synchronized mergeFrom) is clearly documented.