#70: Add v2 UDF protocol design - #18
Conversation
Make the high-level column definition the single source for Exasol type parameters and link to its Call Metadata contract from the Arrow mapping document. Remove repeated property ownership text and legacy exasol field metadata from the mapping examples. Keep only conversion-specific GeoArrow and interval extension metadata, and update validation assertions and rendered mapping documentation accordingly.
| Receive path: | ||
|
|
||
| 1. bytes on the socket | ||
| 2. one length-prefixed and decoded `Frame` |
There was a problem hiding this comment.
What is the size of prefix length? Didn't find it anywhere
|
|
||
| Receive path: | ||
|
|
||
| 1. bytes on the socket |
|
One improvement could be to move the SQL Column Schema from JSON to the DataSchema. @tkilias |
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
tkilias
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bfa9b0731
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - the first record batch may be sent without a received `Next(...)` | ||
| - subsequent batches require an available budget from a previously received `Next(...)` | ||
| - the sender may send less than the hinted budget and may send multiple batches while budget remains | ||
| - an indivisible batch may exceed the remaining budget |
There was a problem hiding this comment.
When the peer is untrusted or memory-constrained, allowing the first batch without credit and permitting any indivisible batch to exceed the remaining budget lets a sender announce arbitrarily large buffers and force the receiver to allocate/read them despite granting zero or minimal credit. This defeats the stated resource-safety purpose of Next(byte_budget) and can turn one stream into a memory-exhaustion path; define a negotiated absolute batch limit and require every batch, including the first, to stay within granted or pre-negotiated credit.
Useful? React with 👍 / 👎.
| table CloseControlMessage { | ||
| close_call: CloseCall; | ||
| close_connection: CloseConnection; |
There was a problem hiding this comment.
A CloseControlMessage can contain both fields simultaneously, and the new protocol test actually constructs that combination on stream 7. However, CloseCall is valid only on a nonzero call stream while CloseConnection is valid only on stream 0, so no stream_id makes such a message valid and receivers are left with conflicting call-versus-connection shutdown semantics. Represent these as mutually exclusive union variants, or explicitly reject the combined state rather than describing it as supported.
Useful? React with 👍 / 👎.
| "size": { "type": "integer", "minimum": 0, "maximum": 4294967295, "description": "Existing size property: character length for CHAR/VARCHAR or declared hash size for HASHTYPE." }, | ||
| "size_unit": { "enum": ["BYTE", "BIT"], "description": "HASHTYPE size unit; required for hash declarations." }, |
There was a problem hiding this comment.
For a HASHTYPE column that omits size or size_unit, this schema still accepts the object because only name, type, and type_name are required, despite size_unit being documented here as required. The consumer then cannot derive the required FixedSizeBinary byte width without parsing type_name, contrary to the dedicated-property contract; add type-conditional requirements here and to the duplicated import-specification column definition.
Useful? React with 👍 / 👎.
| [type_mapping.md](type_mapping.md). The shared column-definition contract is defined in | ||
| [column.schema.json](../../../../../udf-runner-cpp/v2/json_schema/column.schema.json) and is referenced by both column | ||
| metadata and import specifications. |
There was a problem hiding this comment.
The documented column.schema.json contract does not exist in the repository: a repo-wide file search finds only column_metadata.schema.json, while the column definitions are duplicated inline in that schema and import_specification.schema.json. Consequently this link is broken and protocol implementers cannot consume the claimed shared contract; add and reference the shared schema, or remove this claim and link to the actual definitions.
Useful? React with 👍 / 👎.
This branch is waiting to be deployed
2 participants
Summary
Validation
git diff --checkbazel build --verbose_failures --config clang-format //...bazel test --spawn_strategy=local --test_output=errors //:udf_protocol_testpoetry check --lockFixes #70