-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add static getter for TableDefinition in KafkaTools #5956
feat: Add static getter for TableDefinition in KafkaTools #5956
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
* @return A TableDefinition derived from the input Properties and KeyOrValueSpec instances | ||
*/ | ||
@SuppressWarnings("unused") | ||
public static TableDefinition getTableDefinition( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should explicitly call out the potential statefulness of KeyOrValueSpec
. If the caller is going to rely on the correctness of this method, they must call consume with the exact same instances of keySpec / valueSpec.
ie, the following is incorrect:
TableDefinition tableDefinition = getTableDefinition(properties, null, avroSchema("MyValueSchema"));
Table table = consume(properties, ..., avroSchema("MyValueSchema"), ...)
it must be
KeyOrValueSpec myValueSpec = avroSchema("MyValueSchema");
TableDefinition tableDefinition = getTableDefinition(properties, null, myValueSpec);
Table table = consume(properties, ..., myValueSpec, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rather, we should add documentation about the "guarantees" between getTableDefinition and consume; in what conditions is the table definition from getTableDefinition guaranteed to be the table definition of consume? (In my mind, it should be guaranteed when you use the exact same keySpec / valueSpec due to the statefulness of them.)
Labels indicate documentation is required. Issues for documentation have been opened: Community: deephaven/deephaven-docs-community#300 |
No description provided.