JackieTien97 commented on issue #18428: URL: https://github.com/apache/iotdb/issues/18428#issuecomment-5368135301
Thanks for the detailed proposal and addendum. I reviewed them against the current `master` implementation ([`a110cc0`](https://github.com/apache/iotdb/commit/a110cc024f93c674ddd2dc9cc6d6b496b38ba927)). The duration-pair model and anchor-relative arithmetic are good directions, but I think several points need to be resolved before this becomes the implementation baseline. I consider the first three correctness/compatibility blockers. ### 1. The proposed DST-overlap rule is inconsistent with the existing `GROUP BY TIME` calendar behavior The addendum resolves an overlap with `ZonedDateTime.ofLocal(targetLocal, zone, anchorOffset)`, preferring the original anchor offset when it is valid. However, the current `GROUP BY TIME` month path goes through [`calcPositiveIntervalByMonth`](https://github.com/apache/iotdb/blob/a110cc024f93c674ddd2dc9cc6d6b496b38ba927/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/aggregation/timerangeiterator/AggrWindowIterator.java#L80-L94), whose underlying calendar conversion resolves the resulting local time with the normal `atZone` rule (the earlier offset in an overlap). For example, in `America/New_York`: ```text B = 2024-01-03 01:30 -05:00 E = 10mo target local time = 2024-11-03 01:30 // overlap ``` The addendum's CQ rule selects `-05:00` (`06:30Z`), while the current group-by rule selects `-04:00` (`05:30Z`). A CQ RANGE ending at `06:30Z` combined with `GROUP BY(10mo)` can therefore create a one-hour partial/misaligned bucket. That conflicts with the stated goal that CQ cadence/RANGE and `GROUP BY TIME` use consistent calendar semantics. CQ should reuse exactly the same calendar helper and overlap rule as `GROUP BY TIME`. If the shared rule is intentionally changed, that is a broader behavior change and should be made explicit and tested for both paths. ### 2. The mixed-version policy is not actually fail-closed for requests entering through an old DataNode An old DataNode already accepts `EVERY 1mo`, flattens it to 30 days, and sends only the legacy `i64` fields. Under the proposed rules, a new ConfigNode interprets a request with no structured fields as a valid legacy fixed-only request. Consequently: 1. the old DataNode never performs the new ingress version check; 2. the new ConfigNode cannot tell from the duration fields that the user wrote `1mo`; 3. because the request now looks fixed-only, the calendar-CQ version gate is bypassed. Removing old DataNodes from client routing is an operational prerequisite, not an enforceable protocol guarantee. A new ConfigNode needs an enforceable way to identify this case, for example by parsing the original `req.sql` and rejecting legacy requests whose CQ `EVERY`/`RANGE` contains calendar units, rejecting all legacy-protocol CQ creation during the mixed-version window, or requiring an explicit source-capability/protocol marker. Also, [the legacy duration fields remain required](https://github.com/apache/iotdb/blob/a110cc024f93c674ddd2dc9cc6d6b496b38ba927/iotdb-protocol/thrift-confignode/src/main/thrift/confignode.thrift#L1109-L1119), but the proposal does not define which values a new DataNode sends there when a structured duration has a nonzero month part. A 30-day approximation is unsafe, while zero may break an accidental old reader. The wire contract and the hard reader barrier should both be specified. ### 3. `nextOccurrenceIndex` needs an exact CAS/fencing transition Persisting `nextOccurrenceIndex` is the right model, but "persist progress before scheduling the next occurrence" is not sufficient by itself during a ConfigNode leader change. The current implementation accepts any greater execution time for the same CQ token ([`CQInfo.updateCQLastExecutionTime`](https://github.com/apache/iotdb/blob/a110cc024f93c674ddd2dc9cc6d6b496b38ba927/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/cq/CQInfo.java#L196-L229)), and the callback can continue scheduling after most progress-write failures ([`CQScheduleTask`](https://github.com/apache/iotdb/blob/a110cc024f93c674ddd2dc9cc6d6b496b38ba927/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/cq/CQScheduleTask.java#L287-L315)). Old- and new-leader callbacks can therefore race and derive competing schedule chains. The progress write should carry the CQ generation/token and expected index `n`, and atomically perform: ```text nextOccurrenceIndex: n -> n + 1 ``` only if both still match. If the stored index is already greater than `n`, the callback is stale/duplicate and must not schedule another task. If the write fails, the same `n` should be retried. For `DISCARD`, advancement after a successful `n` should be at least `max(n + 1, lowerBound(callbackTime))`, including clock rollback/equality cases. ### 4. Duration comparison still needs a deterministic, implementable definition "Provable for every applicable calendar position, time zone, and DST transition" is a useful principle, but it does not define the actual comparator or the validation against `continuous_query_minimum_every_interval`. In particular, an existing 28-days-per-month minimum must not be reused as a conservative lower bound: a calendar month crossing a spring-forward transition can be shorter than 28 elapsed days (for example, `27d23h`). Historical zone transitions can be larger as well. Please specify the exact partial order. A simple safe rule for duration-vs-duration checks is component-wise dominance, with calendar-vs-fixed cases rejected when ordering cannot be proven. Validation against the fixed minimum-EVERY configuration needs its own explicit rule so that different implementations or TZDB data do not produce inconsistent acceptance. ### 5. I do not recommend adding CQ-only `month`/`year` aliases in this issue Current Tree SQL duration literals support the existing abbreviations such as `y` and `mo` ([lexer rule](https://github.com/apache/iotdb/blob/a110cc024f93c674ddd2dc9cc6d6b496b38ba927/iotdb-core/antlr/src/main/antlr4/org/apache/iotdb/db/qp/sql/SqlLexer.g4#L1313-L1317)). Table SQL has `YEAR`/`MONTH`, but Table SQL does not currently provide CQ syntax. Allowing `EVERY 1month` while rejecting `GROUP BY(1month)`, date arithmetic, `FILL`, and `SESSION` creates an unnecessary CQ-private dialect. A single CQ could have to write: ```sql RESAMPLE EVERY 1month RANGE 1month ... GROUP BY(1mo) ``` I recommend keeping this change aligned with existing Tree SQL and supporting only `y`/`mo`. Full `year`/`month` aliases should be a separate, consistent Tree SQL grammar enhancement covering all duration positions. The following parts of the proposal look sound and should be retained: - structured `(monthPart, fixedPart)` durations with `y -> 12mo`; - deriving every occurrence and RANGE endpoint from the original boundary, rather than repeated clamped addition/subtraction; - preserving existing persisted CQs as legacy fixed-duration CQs; - persisting the CQ zone, explicit-boundary state, and exact anchor-relative progress; - sending concrete timestamps to DataNodes; - computing timeout from adjacent actual occurrence instants. With the DST alignment, mixed-version ingress hole, and progress CAS semantics addressed, the core design should be in good shape. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
