Skip to content
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

[CT-1998] Improve handling of dictionaries in protobuf messages #6832

Closed
gshank opened this issue Feb 1, 2023 · 1 comment · Fixed by #7190
Closed

[CT-1998] Improve handling of dictionaries in protobuf messages #6832

gshank opened this issue Feb 1, 2023 · 1 comment · Fixed by #7190
Assignees
Labels

Comments

@gshank
Copy link
Contributor

gshank commented Feb 1, 2023

In pull request #6820 we implemented protobuf message versions of nodes and node configs. However the match between Python dictionaries that allow values of any type and protobuf maps isn't straightforward. As a result we have temporarily forced a number of dictionaries into string/string dictionaries.

We should look at other options for this and improve the representation of dictionaries in protobuf messages. In particularly this applies to the "meta" dictionary, which is already included in 1.4 in the node_info structures in the logs.

In addition to "meta", the following dictionaries are affected:

  • in NodeConfig - persist_docs, quoting, column_types, grants
  • in ColumnInfo - _extra
  • in various nodes - unrendered_config
  • in TestMetadata - kwargs
  • in SourceDefinitino - source_meta
@github-actions github-actions bot changed the title Improve handling of dictionaries in protobuf messages [CT-1998] Improve handling of dictionaries in protobuf messages Feb 1, 2023
@gshank gshank self-assigned this Feb 17, 2023
@jtcohen6
Copy link
Contributor

Copying from #6803 (comment)

Next steps here:

  • Check out what Struct can do
  • See what map<str, Any> can do
  • Both have their limitations
  • Pick one, and go implement

Closing #6581 and #6803 in favor of this one — more context in the discussions there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants