storage: Export primary ingestion collection #30991
Merged
Conversation
This file contains 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
Previously, when the `force_source_table_syntax` flag was enabled, the primary source ingestion collection was not included in the source exports. This would cause the primary source ingestion collection's upper and since to be stuck at 0, and it would break some existing code. This commit always includes the primary ingestion collection in the source exports. However, when the `force_source_table_syntax` flag is enabled, then the source export details are set to `SourceExportDetails::None`. The result is that all source types with the flag enabled behave similarly to how multi-output sources behave with the flag disabled in regard to the primary ingestion collection. Specifically, their upper's and since's move forward in time and querying them returns an empty result. A downside of this commit is that a source ingestion is always scheduled with the flag enabled, even if there are no table exports. Previously, they would only be scheduled if there were table exports. Works towards resolving #MaterializeInc/database-issues/issues/8620
petrosagg
reviewed
Jan 15, 2025
| @@ -1549,6 +1550,19 @@ where | |||
| if let DataSource::Ingestion(ingestion) = &mut description.data_source { | |||
| if let Some(export) = ingestion.desc.primary_source_export() { | |||
| ingestion.source_exports.insert(id, export); | |||
| } else { | |||
| let export = SourceExport { | |||
There was a problem hiding this comment.
This looks like the right logic but I wasn't expecting to happen here. Can you move this in the planner?
There was a problem hiding this comment.
I tried my best to move this to the planner. Let me know if that's what you had in mind. It involves not removing the primary export though.
There was a problem hiding this comment.
It involves not removing the primary export though.
I wasn't really sure how else to do it in the planner.
…mary-export-details-none
|
@def- what I'd really like for this PR is to take all of the tests that involve the |
|
Looks like key-value load gen sources specifically are broken with the |
|
The problem originates form here: materialize/src/storage/src/source/generator.rs Lines 149 to 162 in 1fd704b We only populate the output map with source exports that have a non Then in the key-value generator code, we assume that the output map is always non-empty, which is not true. So we panic. materialize/src/storage/src/source/generator/key_value.rs Lines 94 to 98 in 1fd704b Hopefully we can just return early if the output map is empty. |
That does not work. For whatever reason that causes the upper to advance to the empty set. If we then try and create a table on the source, it gets stuck at a since and upper of 0. |
|
I tried a couple of quick and dirty fixes for the key-value load generator and nothing worked. I think we should address the issues in a follow-up PR and not this one. I opened the following issue for it: https://github.com/MaterializeInc/database-issues/issues/8904 |
…mary-export-details-none
Of course I immediately thought of something that worked: #31134. I still think it's best saved for it's own PR, so this PR should be ready for review and merge. |
…mary-export-details-none
aljoscha
approved these changes
Jan 22, 2025
| # | ||
|
|
||
| $ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr} | ||
| ALTER SYSTEM SET force_source_table_syntax = true |
There was a problem hiding this comment.
It really is just a plain copy ... 😅
5 tasks
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.
Previously, when the
force_source_table_syntaxflag was enabled, the primary source ingestion collection was not included in the source exports. This would cause the primary source ingestion collection's upper and since to be stuck at 0, and it would break some existing code.This commit always includes the primary ingestion collection in the source exports. However, when the
force_source_table_syntaxflag is enabled, then the source export details are set toSourceExportDetails::None. The result is that all source types with the flag enabled behave similarly to how multi-output sources behave with the flag disabled in regard to the primary ingestion collection. Specifically, their upper's and since's move forward in time and querying them returns an empty result.A downside of this commit is that a source ingestion is always scheduled with the flag enabled, even if there are no table exports. Previously, they would only be scheduled if there were table exports.
Works towards resolving #MaterializeInc/database-issues/issues/8620
Motivation
This PR adds a known-desirable feature.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.