fix(data-imports): don't fail imports on a failed row count refresh - #74882
Draft
posthog[bot] wants to merge 1 commit into
Draft
fix(data-imports): don't fail imports on a failed row count refresh#74882posthog[bot] wants to merge 1 commit into
posthog[bot] wants to merge 1 commit into
Conversation
The cumulative row-count refresh runs after the sync has already written its data, so a failing count query took down an otherwise successful import. Keep the previous row_count and log instead. Also map ClickHouse type-mismatch errors in ExtractErrors, so users see that files in the table disagree on a column's type rather than being told to check their URL pattern, format, and credentials. Generated-By: PostHog Code Task-Id: 1956f774-0a8f-48ef-a639-9544bbe67356
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Warehouse imports were failing after the data had already landed, and the error told customers to check their bucket credentials.
Two things go wrong on the same path. After a sync writes its data,
_validate_and_updaterefreshes the row count for cumulative schemas by callingDataWarehouseTable.get_count(). That count runs chdb first (30s hard timeout), then falls back to the ClickHouse cluster over the Delta parquet glob with no explicitstructure, so ClickHouse infers the schema across files. When chunks disagree on a column's physical type, a plainSELECT count()fails withCannot convert string '...' to type Date._safe_expose_ch_errorfinds no matching entry inExtractErrors, so it falls through to the catch-all: "check that the files URL pattern, file format, and credentials are correct". Credentials are the one thing that isn't wrong.Both legs of the fallback are failing in the wild - some occurrences hit the chdb timeout, others the ClickHouse type mismatch - across several teams over the last month.
Changes
row_countis kept and a warning is logged instead of sinking the job. Applied to bothvalidate_schema_and_update_tableandregister_cdc_companion_table, which had the same shape.ExtractErrorsso the surfaced message names the real cause (files disagreeing on a column's type) and suggests a resync.Note
Passing an explicit
structuretobuild_function_callfromget_count()would fix the inference fragility at the root. That's a bigger change and worth weighing separately, so it isn't here.How did you test this code?
Automated tests only, all run locally and passing:
products/warehouse_sources/backend/tests/test_table.py- 12 passed, including the new case asserting the real production message maps to the type-mismatch copy instead of the credentials catch-all. Regression it catches: dropping theExtractErrorsentry silently sends users back to checking credentials that were never the problem.products/warehouse_sources/backend/temporal/data_imports/pipelines/test_pipeline_sync.py- 16 passed, including the newTestValidateSchemaAndUpdateTable, which covers the failing count (job completes, previousrow_countkept, newqueryable_folderstill saved) and the happy path (count refreshed from the table, not taken from the run's delta). Regressions they catch: a count query raising again taking down the whole activity, or the new guard swallowing a successful refresh.No manual testing - I didn't reproduce a real S3 sync. Note that the Django test job is skipped on draft PRs, so these hadn't run in CI at the time of writing.
Automatic notifications
Docs update
No user-facing docs cover this error string.
🤖 Agent context
Autonomy: Fully autonomous
Written by Claude (Claude Code) from a PostHog inbox report. Skills invoked:
/writing-user-facing-copyfor the new error message,/writing-testsfor the test decisions.A couple of decisions worth flagging for review:
ExtractErrorskey is the substringCannot convert string. It's broad on its own, but_safe_expose_ch_erroris only reached from the S3 table-function paths (get_count,get_columns, column validation), where that message means the files disagree on a type. Happy to make it narrower if you'd rather anchor onWhile executing ReadFromObjectStorage.safe_expose_ch_error=Falseinside the guard, so the raw ClickHouse error lands in the log rather than the friendly-but-now-irrelevant rewrite._register_companion_synchelper intest_pipeline_sync.py- it's a hand-copied duplicate of the production function, so a test against it would prove nothing about the real code path. The new tests call the realvalidate_schema_and_update_table.Created with PostHog Desktop from this inbox report.