docs: naming rules for Named Structs #688
Conversation
vbarua
requested review from
jacques-n,
cpcloud,
westonpace and
EpsilonPrime
as code owners
vbarua
force-pushed
the
vbarua/named-struct-naming-rules
branch
from
28ed4bc to
031586c
Compare
vbarua
force-pushed
the
vbarua/named-struct-naming-rules
branch
from
031586c to
dd6c0c1
Compare
site/docs/types/named_structs.md
Outdated
| ``` | ||
| has 2 names, one for each of its inner fields. | ||
|
|
||
| ### Nested Structs |
There was a problem hiding this comment.
Basically this applies to any struct in any complex type (maps, lists, and structs). Not sure if introducing that term makes the document simpler.
There was a problem hiding this comment.
Switched to Compound Types which is the language we use in the docs: https://substrait.io/types/type_classes/#compound-types
site/docs/types/named_structs.md
Outdated
| a b | ||
| struct<i64, fp64> | ||
|
|
There was a problem hiding this comment.
Is it this project's style to put the names above the types? Or are a and b supposed to be metasyntactic variables that aren't referenced here? I would find something like struct<a: i64, b: fp64> much less confusing. :)
There was a problem hiding this comment.
or, looking at the later examples, is the user responsible for flattening the tree of names?
There was a problem hiding this comment.
+1 for the struct<a: i64, b: fp64> form, maybe with bolded names
There was a problem hiding this comment.
The NamedStruct messages consists of a Type.Struct message, which only contains type information, no names, and a names field which is the depth-first encoded list of names associated with the struct.
Is it this project's style to put the names above the types?
The Struct message itself does not contain any name information. The format I'm using the type names as defined in https://substrait.io/types/type_classes/#compound-types to represent the types.
I'm wary of struct<a: i64, b: fp64> because it makes it look like the Structs may have names. There actually is a NSTRUCT pseudo-construct for this, but it's not really used anywhere (that I know of).
There was a problem hiding this comment.
And changed my mind after more feedback. Added a note to the top of the page and inlined the names.
There was a problem hiding this comment.
To be clear, I wasn't arguing for a change in the internal representation... It's just that I found this particular part of Substrait quite confusing, so other passersby are likely to as well. :)
There was a problem hiding this comment.
I'm certain that it would be duplicative, but it would probably help to have a paragraph or two--or at least a very specific hyperlink--about Substrait's approach here. With that context, the separate listings of types and names makes more sense...
I might even recommend going so far as to add vertical tree visualizations of the nested structures, to make the depth-first part even clearer... but pretty soon you're talking about real effort ;)
Co-authored-by: Arttu <[email protected]>
Co-authored-by: Arttu <[email protected]>
No description provided.