Skip to content

Conversation

@lukaszgryglicki
Copy link
Member

cc @mlehotskylf @ahmedomosanya

Signed-off-by: Lukasz Gryglicki [email protected]

Assisted by OpenAI

Assisted by GitHub Copilot

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Walkthrough

A cutoff-based gating mechanism is introduced for Fivetran-synced timestamps in the backfill process. A new constant and helper function determine if timestamps precede a cutoff date; the Snowflake pass conditionally skips using _FIVETRAN_SYNCED for created and modified timestamps when before the cutoff to maintain consistent ordering.

Changes

Cohort / File(s) Summary
Fivetran Cutoff Gating Logic
cla-backend-go/cmd/signatures_timestamp_backfill/main.go
Adds fivetranCutoffDate constant and isFivetranBeforeCutoff() helper to gate _FIVETRAN_SYNCED usage. Introduces skipFivetran pre-check in Snowflake pass; prevents using _FIVETRAN_SYNCED for created timestamps and modifies logic for selecting modified timestamps based on cutoff evaluation. Includes conservative error handling in helper function.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15–20 minutes

  • New constant and helper function with conservative fallback logic require verification of cutoff semantics
  • Modified timestamp selection logic in the Snowflake pass involves conditional branching that needs careful review to ensure monotonic ordering is preserved
  • Single file scope but logic changes span multiple decision points in the backfill flow

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The description addresses reviewers and includes metadata (signed-off-by, assistance credits) but lacks meaningful explanation of the changeset's purpose or technical details. Add a brief explanation of what the change accomplishes and why the March 9, 2024 cutoff date is necessary.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and specifically describes the main change: implementing a cutoff date (March 9, 2024) to prevent using older Fivetran synced timestamps.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch unicron-4862-use-only-newer-fivetran-sync-dates

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
cla-backend-go/cmd/signatures_timestamp_backfill/main.go (1)

40-41: Add documentation for the cutoff date.

The constant fivetranCutoffDate is set to March 9, 2024, but there's no explanation for why this specific date is significant. Consider adding a comment explaining the reason for this cutoff (e.g., data quality issues, migration event, system change, etc.) to help future maintainers understand the context.

Apply this diff to add documentation:

-	// cutoff date for _FIVETRAN_SYNCED usage
-	fivetranCutoffDate = "2024-03-09T00:00:00Z"
+	// cutoff date for _FIVETRAN_SYNCED usage
+	// Fivetran synced timestamps on or before this date are unreliable and should not be used.
+	// TODO: Add ticket reference or explanation for why March 9, 2024 is the cutoff
+	fivetranCutoffDate = "2024-03-09T00:00:00Z"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8bdae40 and bc7384b.

📒 Files selected for processing (1)
  • cla-backend-go/cmd/signatures_timestamp_backfill/main.go (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Agent
  • GitHub Check: CodeQL analysis (go)
  • GitHub Check: build-test-lint
  • GitHub Check: cypress-functional
🔇 Additional comments (2)
cla-backend-go/cmd/signatures_timestamp_backfill/main.go (2)

552-576: LGTM! Cutoff gating correctly implemented.

The Snowflake pass now correctly checks if _FIVETRAN_SYNCED timestamps fall on or before the cutoff date and skips them accordingly. The logic ensures:

  • Created timestamps only use Fivetran data when !skipFivetran (line 559)
  • Modified timestamps fall back to other sources when skipFivetran is true (line 574)
  • Appropriate debug logging is added for visibility (line 555)

The fallback behavior for both created and modified timestamps is preserved.


1079-1090: Consider the trade-off of conservative error handling.

The function returns true (skip Fivetran data) when parsing fails (lines 1083, 1087). This is a conservative approach that errs on the side of caution, but it means that:

  • Transient parse errors will cause valid Fivetran timestamps to be skipped
  • Unexpected timestamp formats will be rejected even if they're after the cutoff

This might be the intended behavior for data integrity, but consider whether logging these parse failures as warnings would help identify systematic issues with timestamp formats.

The core comparison logic at line 1089 (!t.After(cutoff)) is correct and properly implements "skip if timestamp ≤ cutoff".

Do you want to add warning logs for parse failures to help identify potential data quality issues?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request adds logic to exclude Fivetran synced timestamps that are on or before March 9, 2024, from being used in the signature timestamp backfill process. This prevents potentially unreliable historical Fivetran data from affecting timestamp corrections.

Key Changes:

  • Added a cutoff date constant (fivetranCutoffDate = "2024-03-09T00:00:00Z")
  • Implemented isFivetranBeforeCutoff() function to check if a timestamp should be excluded
  • Modified the snowflakeFix() function to skip Fivetran timestamps when they fall on or before the cutoff date

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lukaszgryglicki lukaszgryglicki merged commit 35c84d7 into dev Nov 19, 2025
10 of 12 checks passed
@lukaszgryglicki lukaszgryglicki deleted the unicron-4862-use-only-newer-fivetran-sync-dates branch November 19, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants