refactor(config): decouple worker-core config types from amp-config schemars#1873
Open
refactor(config): decouple worker-core config types from amp-config schemars#1873
Conversation
…chemars Duplicate ParquetConfig, ConfigDuration, and nested config types in amp-config with schemars support, breaking the feature chain amp-config[schemars] → amp-worker-core/schemars → common/schemars. Internal crates no longer carry the schemars dependency. Closes #1790
Define a proper Compression enum in amp-config with a curated subset of supported algorithms (zstd, lz4, gzip, brotli, snappy, uncompressed). Validates at the config boundary via FromStr, removing the need for worker-core to expose parse_compression in its public API.
…ring Reorder worker_core.rs to follow rust-modules-members guideline: main type (ParquetConfig) first, then dependencies in order, private helpers last. Alphabetize Cargo.toml dependencies and regenerate config schema.
Eliminates the .expect() in the From<&Compression> impl by storing parquet_basic::ZstdLevel in the Zstd variant. Validation now happens in FromStr where errors can be returned properly.
mitchhs12
changed the title
…message Remove unnecessary pub from private SizeLimitHelper fields. Forward the error from ZstdLevel::try_new directly instead of hardcoding the valid range.
…onfig paths - Remove serde::Deserialize from all worker-core config types; amp-config is the sole deserialization boundary - Create local ZstdLevel newtype wrapping parquet's ZstdLevel, validated at parse time against parquet's own constraints (zero unwrap/expect) - Slim amp-config SizeLimitConfig to user-configurable fields only; runtime fields (file_count, generation, blocks) default during From - Revert parquet_opts signature to &ParquetConfig, removing unnecessary impl Into + clone at call sites - Replace Duration::from_secs_f64 with try_from_secs_f64 to prevent panic on negative/NaN/infinite duration input - DRY up SizeLimitHelper deserialization via shared deserialize_with_base - Document Compression parsing semantics and gzip/brotli level behavior - Remove redundant error message wrapping in FromStr for Compression - Update ampd.spec.json to reflect slimmed SizeLimitConfig schema
Remove default_compression(), default_cache_size_mb(), default_max_row_group_mb() from worker-core (no longer needed without serde) and default_upper_limit() from amp-config (was just Self::default()). Inline their values directly at the call sites.
Accept impl Into<ParquetConfig> per issue #1790 guidance, so callers can pass either worker-core's own type or a foreign type that implements Into<ParquetConfig>. Call sites now clone before passing ownership.
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
1 participant
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.
Note: WIP
Summary
ParquetConfig,ConfigDuration, and nested config types (CollectorConfig,CompactorConfig,CompactionAlgorithmConfig,SizeLimitConfig) inamp-configwith schemars supportamp-config[schemars]→amp-worker-core/schemars→common/schemarsschemarsdependency from bothworker-coreandcommonworker-core::parquet_opts()now acceptsimpl Into<ParquetConfig>pub fn parse_compressionto worker-core so amp-config can validate compression strings without depending ondatafusiondirectly — needs reviewSizeLimitConfigdeserialization helpers (~55 lines) are duplicated verbatim from worker-core into amp-config — needs review on whether to deduplicate by making worker-core's versionspubCloses #1790