[SQL] Connector transport/format validation in the SQL compiler#6538
Conversation
gz
left a comment
There was a problem hiding this comment.
can we drop the validation in the pipeline-manager platform?
|
How are we going to maintain this code in sync with Rust? |
|
There’s documentation. But in general we don’t have to; these are just warnings. The runtime connectors have the final word.
|
mythical-fred
left a comment
There was a problem hiding this comment.
Nice piece of work — the dispatch + Jackson POJO design is straightforward and the test surface is wide. A few schema-drift and behavior issues worth fixing before merge; calling them out inline.
Two themes worth flagging up front, not as blockers:
-
Schema-drift contract. Spot-checked Kafka, S3, DeltaTableReader, Iceberg, Postgres, DynamoDB, Json, Nats, PubSub against the Rust serde structs — schemas line up. But there is no mechanism to keep them in sync. Worth tracking as follow-up: a code-generator (Java POJOs from
feldera-typesschemars, or a CI cross-check that the Java field set is a subset of the Rust struct field set) would close the drift hole permanently. Today every renamed/added Rust field is a silent miss in the SQL compiler. -
Fail-open contract. Validator emits warnings (not errors), unknown formats/transports fall through the
default:arm silently, and config-level deserialization failures (other thanUnrecognizedPropertyException) also degrade to warnings. That is the right call for forward-compat with the runtime — but it means the Java check is genuinely best-effort, as the PR description says. Worth documenting that explicitly in the README so users (and connector authors) know not to rely on the SQL compiler for correctness; the runtime remains the source of truth.
Individual issues inline.
| "table name \"" + table + "\" is " + table.length() | ||
| + " character(s); DynamoDB requires 3–255 characters"); | ||
| ok = false; | ||
| } else if (!table.chars().allMatch(c -> Character.isLetterOrDigit(c) || c == '_' || c == '-' || c == '.')) { |
There was a problem hiding this comment.
Drifts from the Rust validator. Rust uses c.is_ascii_alphanumeric() (see crates/feldera-types/src/transport/dynamodb.rs); Character.isLetterOrDigit is true for any Unicode letter or digit, so a table name like tableñ or テーブル passes the SQL compiler and then fails at runtime. Switch to (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') (or Character.isLetter(c) && c < 0x80 style) to match Rust exactly.
|
|
||
| @Override | ||
| public boolean validate(ConfigReporter reporter) { | ||
| return true; |
There was a problem hiding this comment.
uri and query are required in the Rust struct (no #[serde(default)]), but validate() returns true unconditionally. A config with "uri": "" or missing query slips through here. Follow the S3 / Kafka pattern and emit warnPath("uri", ...) / warnPath("query", ...) when blank.
|
|
||
| @Override | ||
| public boolean validate(ConfigReporter reporter) { | ||
| return true; |
There was a problem hiding this comment.
Same as PostgresReaderConfig: uri, publication, and source_table are required in Rust but validate() does nothing. Empty strings should be flagged.
| /** Configuration for the Delta Table input transport. */ | ||
| public class DeltaTableReaderConfig implements IValidateConfig { | ||
| @JsonProperty("uri") | ||
| public String uri = ""; |
There was a problem hiding this comment.
uri is required in feldera-types/src/transport/delta_table.rs (no #[serde(default)]). validate() further down checks mode but never uri.isBlank(), so a missing/empty uri is a silent pass.
| validateConfig(formatConfig, outerJson, configPointer, | ||
| outerStart, RawParserConfig.class, reporter); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
Unknown format names fall through silently. The same is true for unknown transport names in validateTransportConfig (default: break). That is deliberate fail-open (forward-compat with newer runtime), and it is the right call — but combined with the rest of this file it means the SQL compiler can be completely silent on a typo like "name": "jsno". Worth at least an inline comment here saying "intentionally silent: validation is best-effort, runtime is the source of truth," and a mention of this fail-open contract in the README so connector authors and users know not to rely on it.
| @JsonProperty("default") Default, | ||
| @JsonProperty("debezium_mysql") DebeziumMySql, | ||
| @JsonProperty("debezium_postgres") DebeziumPostgres, | ||
| @JsonProperty("snowflake") Snowflake, |
There was a problem hiding this comment.
Nit: the Rust enum has no rename_all and ClockInput has no explicit #[serde(rename = ...)], so by default serde serializes it as "ClockInput" — matches. But this is the only mixed-case key in the table; a brief comment /* no serde rename in Rust → variant name as-is */ would save the next reader a trip to the Rust source.
|
|
||
| #### When you add a field to an existing Rust struct | ||
|
|
||
| Find the matching POJO class in `config/` and add the field following the |
There was a problem hiding this comment.
Worth documenting the fail-open contract explicitly: (a) unknown transport/format names are silently skipped by the SQL compiler, (b) Jackson deserialization errors surface as warnings not errors, (c) @JsonAnySetter keys are not validated at all. Future contributors will otherwise assume "SQL compiler accepted it → it works." Also: this README does an excellent job of explaining mechanics, but nowhere says "the runtime is still the source of truth — don't remove Rust-side validation when you add a Java mirror." Worth a one-liner since the whole point of the doc is to guide future connector additions.
|
|
||
| /** Absorbs flattened {@code kafka_options} keys so they are not rejected as unknown. */ | ||
| @JsonAnySetter | ||
| public void setKafkaOption(String key, Object value) {} |
There was a problem hiding this comment.
@JsonAnySetter here matches the Rust #[serde(flatten)] kafka_options: BTreeMap, but it accepts any unknown key, so a misspelling like "toopic": "foo" is absorbed by setKafkaOption rather than warned on. The same applies to DeltaTableReaderConfig, IcebergReaderConfig, and KafkaOutputConfig. There is no clean fix in pure Jackson (you would need a denylist of declared field names plus a regex for valid Kafka keys), so just acknowledge it: a comment here, and a line in the README under "Currently validated connectors" noting that flattened string-map configs cannot detect typos.
|
The warnings are useful, especially for unknown fields. Yes, the main question is how how effort it will be to keep it in sync with both the Rust code and its own documentation. The PR basically duplicates all the Rust structs into Java classes. It seems likely they will drift -- are they going to be kept in line with AI tools?
The pipeline-manager, does the same deserialization as the pipeline, so it's the best check of "will it work". It is optional though, and if this is kept in-sync, it would be a good replacement to get rid of that task of the manager. |
|
I plan to remove the validation from the pipeline manager after this is merged |
ec3ea4c to
b055b5c
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
Re-review on b055b5c — my three earlier blockers (Postgres reader/CDC required-field checks, Delta uri.isBlank, Dynamo char check) are all addressed. Thanks.
One new issue introduced in the Dynamo fix below, otherwise this version looks good to me. Not blocking the PR over it, but please fix in a follow-up commit before merge.
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
Fixes #4656
Checklist
This PR has used Claude to migrate most of the connector validation from Rust to Java. The benefit is that one gets much faster connector validation. The SQL compiler validation is still best-effort, so it's possible to get connector errors at runtime, but the SQL compiler covers a very large surface. I think that the connectors still independent validation, though.
There connector readme has been updated to describe how new connectors are added to the compiler.
The Java validation code is relatively simple, it relies on a serde-like mechanism to validate the JSON, by declaring lots of Java classes that describe the schema of the configuration and a switch statement on the connector type.