DaZuiZui commented on issue #18428: URL: https://github.com/apache/iotdb/issues/18428#issuecomment-5251660178
## Implementation Approach Addendum: Correctness and Compatibility Constraints Based on the Issue Acceptance Criteria This comment is a normative addendum to the original implementation proposal: https://github.com/apache/iotdb/issues/18428#issuecomment-5235636695 The user-visible behavior described in the issue is the sole source of truth for acceptance. If this addendum conflicts with the original proposal, this addendum takes precedence. This addendum does not replace the core design of the original proposal. The following ideas remain unchanged: - Use structured calendar durations. - Calculate each occurrence from the original `BOUNDARY` using `B + n × E`. - Calculate RANGE endpoints from the original `BOUNDARY` in duration-vector space. - Persist the CQ `ZoneId`. - Preserve legacy fixed-duration behavior for existing CQs without silently migrating them. - Continue sending only concrete `startTime` and `endTime` values to the DataNode. The following constraints are required to prevent silent semantic degradation, DST drift, mixed-version failures, and scheduling drift after recovery. ### 1. The supported scope must follow the issue CQ `EVERY` and `RANGE` must support: - Month units: `mo` and `month` - Year units: `y` and `year` - Case-insensitive unit names - `1y` as the equivalent of 12 calendar months - No new plural forms such as `months` or `years`, because the issue does not require them If the current shared `DURATION_LITERAL` cannot provide the complete set of aliases only for CQ, the implementation must choose one of the following approaches: 1. Extend the shared duration grammar and add regression coverage for the other Tree SQL features that use durations. 2. Add a CQ-specific duration parser rule. The final acceptance scope of this issue must not exclude `month` and `year` merely because the current grammar supports only abbreviated forms. ### 2. Calendar durations must never be flattened CQ must use a structured duration representation: ```text CQDuration { long monthPart long fixedPart } ``` Where: - `monthPart` represents a number of calendar months. - `fixedPart` represents fixed ticks in the current timestamp precision. - `1y` is parsed as `monthPart = 12`. - `d` and `w` continue to represent fixed durations of 24 hours and 7 × 24 hours, respectively. - Calendar months are applied first, followed by fixed ticks as an elapsed duration. - The textual order of components does not change the normalized result. No parsing, RPC, persistence, recovery, or scheduling path may convert one calendar month into a fixed 30-day duration or one calendar year into a fixed 365-day duration. All duration arithmetic must detect overflow, including: - Accumulating month components - Computing `years × 12` - Multiplying a duration by an occurrence index - Adding or subtracting duration vectors - Adding or subtracting timestamps The implementation should use `Math.addExact`, `Math.subtractExact`, and `Math.multiplyExact`. It must not rely directly on duration multiplication that can overflow silently. ### 3. `BOUNDARY` must preserve the exact instant The persisted `BOUNDARY` representation must include: ```text boundaryInstant zoneId explicitBoundary ``` Calendar arithmetic must satisfy: ```text calendarApply(B, zero, Z) == B ``` The recommended calculation is: ```text calendarApply(B, D, Z): anchor = instant(B).atZone(Z) monthApplied = anchor.plusMonths(D.monthPart) result = ticks(monthApplied.toInstant()) + D.fixedPart ``` The implementation must not convert `BOUNDARY` to an offset-free `LocalDateTime` and then call `atZone()` to convert it back to an instant. That approach can lose the original offset during a DST overlap and may cause `calendarApply(B, 0) != B`. DST resolution rules are defined as follows: - If a generated local time falls into a DST gap, move it forward by the length of the gap and use the first valid time. - If a generated local time falls into a DST overlap, preserve the original `BOUNDARY` offset when that offset is valid; otherwise, select the earlier offset. - A `BOUNDARY` containing an explicit offset represents an exact instant. - A `BOUNDARY` without an offset must be resolved using the `ZoneRules` for the target date and time. It must not use the offset corresponding to the current wall-clock time. - If a `BOUNDARY` without an offset falls into a gap or overlap, return an explicit semantic error and require the user to provide an offset. When `BOUNDARY` is omitted for a calendar `EVERY`, use local `1970-01-01 00:00:00` in the CQ `ZoneId`, resolve it once using `ZoneRules`, and persist the resulting exact instant. An explicit `BOUNDARY 0` must continue to mean the Unix epoch instant and must not be confused with an omitted `BOUNDARY`. ### 4. Scheduling and RANGE must always be calculated from the original anchor The nth occurrence is defined as: ```text executionTime(n) = calendarApply(B, multiplyExact(E, n), Z) ``` The following recurrence is forbidden: ```text executionTime(n + 1) = executionTime(n) + everyInterval ``` The previous occurrence may already have been clamped at the end of a month, so calculating the next occurrence from it can cause date drift. The schedule must satisfy: ```text executionTime(n + 1) > executionTime(n) ``` RANGE endpoints retain the boundary-anchored duration-vector semantics: ```text startTime(n) = calendarApply(B, n × E - startOffset, Z) endTime(n) = calendarApply(B, n × E - endOffset, Z) ``` The internal duration-vector representation must support negative month or fixed components produced by calculations, while duration components supplied by users must remain non-negative. When `RANGE == EVERY`, the implementation must satisfy: ```text startTime(n) == executionTime(n - 1) endTime(n) == executionTime(n) ``` This guarantees contiguous adjacent windows across month ends, leap days, and DST transitions. ### 5. The occurrence index is the authoritative scheduling progress for a calendar CQ The persisted state of a structured calendar CQ must include at least: ```text anchor zoneId everyDuration startOffset endOffset explicitBoundary nextOccurrenceIndex ``` When creating a CQ: ```text firstIndex = findFirstOccurrenceNotBefore(now) nextOccurrenceIndex = firstIndex ``` When scheduling it: ```text executionTime = calendarApply( anchor, everyDuration × nextOccurrenceIndex, zoneId) ``` After successful execution: ```text BLOCKED: newNextIndex = currentIndex + 1 DISCARD: newNextIndex = max( currentIndex + 1, findFirstOccurrenceNotBefore(currentTime)) ``` Calendar occurrence lookup should use an estimate followed by correction, or a bounded binary search. It must not iterate month by month starting from 1970. A new consensus update should be introduced, or the existing update should be replaced with an equivalent structure: ```text UpdateCQProgressPlan { cqId cqToken expectedCurrentIndex newNextOccurrenceIndex } ``` The consensus update succeeds only when all of the following conditions hold: - `cqToken` matches. - The currently persisted index equals `expectedCurrentIndex`. - The new index is strictly greater than the current index. This prevents a stale leader or stale scheduling task from overwriting progress already persisted by the new leader. The following paths must all be updated consistently: - `AddCQPlan` - `CreateCQProcedure` - `CQEntry` - `CQScheduleTask` - `UpdateCQLastExecTimePlan` - Related Plan and Procedure serialization - CQ snapshots After a ConfigNode restart, leader switch, or Procedure recovery, scheduling must resume directly from `nextOccurrenceIndex`. Calendar CQs must not continue using: ```text firstExecutionTime - everyInterval lastExecutionTime + everyInterval ``` A legacy `CQEntry` without an occurrence index must continue using the legacy fixed-duration scheduling path and must not be migrated automatically to a calendar CQ. ### 6. Mixed-version behavior must fail closed Introduce a cluster capability flag: ```text CQ_CALENDAR_DURATION_V1 ``` This capability may be enabled only after every registered ConfigNode and DataNode supports structured CQ durations. Both the DataNode and ConfigNode must enforce the capability: - The DataNode checks the capability before accepting a calendar CQ. - The ConfigNode checks it again before persisting a calendar CQ. - If the capability is not enabled, the calendar CQ is rejected explicitly. - The request must never fall back to a flattened 30-day or 365-day duration. Add the following fields to `TCreateCQReq`: ```text durationFormatVersion structuredEveryDuration structuredStartOffset structuredEndOffset explicitBoundary ``` A new DataNode should always send `durationFormatVersion` and structured durations when creating a CQ, including fixed-only CQs. The existing required `i64` fields may remain for wire compatibility, but the following rule must apply: ```text When durationFormatVersion is present, the structured fields are the only authoritative values. If the structured fields are missing, incomplete, or invalid, the request must be rejected. The implementation must not fall back to the legacy i64 fields and continue creating the CQ. ``` For live creation requests: - Calendar CQs must not be created during a mixed-version upgrade. - A new ConfigNode must not interpret a live calendar request without structured duration fields as a fixed-duration CQ. - If a reliable capability handshake cannot be provided, `CREATE CQ` should be disabled during the mixed-version phase, or the feature must explicitly require all ConfigNodes and DataNodes to be upgraded before it can be used. - Calendar CQs must not be claimed as compatible with old DataNodes or old ConfigNodes. For existing CQs: - An old `CQEntry` without structured durations is loaded as a legacy fixed-duration CQ. - Even if its original SQL contains `mo` or `y`, it must not be reparsed or migrated automatically. - Users explicitly opt into calendar semantics by dropping and recreating the CQ. Snapshots must use a two-phase strategy: - New versions must be able to read both v1 and v2 snapshots. - During a mixed ConfigNode upgrade, ConfigNodes continue writing v1 snapshots and structured calendar CQ creation remains disabled. - Writing v2 snapshots is allowed only after all ConfigNodes have been upgraded. - A v2 snapshot stores the structured durations, anchor, `ZoneId`, `explicitBoundary`, and occurrence index. ### 7. Duration comparison must not use fixed-day approximations The duration comparison function should return: ```text LESS EQUAL GREATER AMBIGUOUS ``` The rules are: - Identical normalized structures: `EQUAL` - Equal `monthPart` values: compare `fixedPart` - Two pure fixed durations: compare their ticks - Two pure calendar durations: compare their month counts - If the ordering can be proven for all relevant dates, time zones, and DST transitions, the values may be compared - Otherwise, return `AMBIGUOUS` and report an explicit semantic error For example: ```text 1mo3d > 1mo 1mo == 1mo 1mo versus 30d -> AMBIGUOUS ``` The same comparison semantics must be used for all of the following validations: - `EVERY > 0` - `startOffset > 0` - `endOffset >= 0` - `startOffset > endOffset` - `startOffset >= EVERY` - `EVERY >= continuous_query_minimum_every_interval` Month values must not be flattened to fixed 30-day durations during validation. ### 8. `TExecuteCQ.timeout` must be converted to milliseconds The actual interval between calendar occurrences is first calculated in timestamp ticks: ```text deltaTicks = executionTime(n + 1) - executionTime(n) ``` Before sending it through `TExecuteCQ`, it must be converted to milliseconds with rounding toward positive infinity: ```text timeoutMs = deltaTicks / ticksPerMillisecond + (deltaTicks % ticksPerMillisecond == 0 ? 0 : 1) ``` The implementation must guarantee: ```text timeoutMs >= 1 ``` It must also handle: - Millisecond, microsecond, and nanosecond timestamp precision - Division and rounding - Overflow when subtracting timestamps - Variations in the actual interval caused by month lengths and DST transitions ### 9. Required acceptance invariants The implementation and tests must verify: ```text 1. calendarApply(B, zero, Z) == B 2. executionTime(n + 1) > executionTime(n) 3. When RANGE == EVERY: startTime(n) == executionTime(n - 1) 4. Before and after a restart, leader switch, or Procedure recovery: nextOccurrenceIndex and its corresponding executionTime are identical 5. A calendar CQ must either: - be created successfully while preserving structured calendar semantics; or - fail with an explicit error It must never be silently converted into a 30-day or 365-day CQ ``` ### 10. Additional required tests In addition to the original test plan, the test suite must cover: - Parser and end-to-end behavior for `mo`/`month` and `y`/`year`. - The issue example that aggregates a complete calendar month with an omitted `BOUNDARY`. - `calendarApply(B, 0) == B`. - January 31, February 29, and months with 28, 29, 30, and 31 days. - An `America/New_York` DST gap. - Both earlier and later offsets in a DST overlap. - A `BOUNDARY` without an offset using the target date’s offset rather than the current date’s offset. - Regression coverage for fixed-only durations. - A calendar `EVERY` inherited from `GROUP BY(1mo/1y)`. - Both `BLOCKED` and `DISCARD` policies. - `DISCARD` when the current time is exactly on an occurrence boundary. - ConfigNode restart and leader switch. - Occurrence-index Plan and Procedure serialization/deserialization. - A real legacy v1 snapshot fixture and a v2 snapshot round trip. - Existing CQs retaining legacy fixed-duration behavior. - Explicit rejection of calendar CQs while the capability is disabled. - Correct timeout units under millisecond, microsecond, and nanosecond precision. - Checked duration arithmetic and timestamp overflow. - One end-to-end integration test using a near-future `BOUNDARY` to verify the complete path: ```text SQL parser → structured TCreateCQReq → ConfigNode persistence → calendar scheduler → concrete startTime/endTime → TExecuteCQ → DataNode query → result written by SELECT INTO ``` With these constraints, the original proposal can remain the implementation baseline while ensuring that the final behavior follows the issue and that a calendar CQ can never be syntactically accepted but silently executed as a fixed 30-day or 365-day CQ. -- 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]
