docs: consistently mark 0 as valid anchor/reference #900
Conversation
benbellick
force-pushed
the
benbellick/consistent-0-anchor
branch
from
c851ebc to
e95927f
Compare
proto/substrait/algebra.proto
Outdated
| // to a scalar function in the associated YAML file. Required; avoid | ||
| // using anchor/reference zero. | ||
| // to a scalar function in the associated YAML file. | ||
| // Required; 0 is a valid anchor/reference. |
There was a problem hiding this comment.
This is the only one I am not sure about... I find the language here a bit confusing. Is it saying the two separate things:
- this field is required, and
- you must not use anchor/reference 0
?
Or is it saying "it is required not to use anchor/reference 0"?
I think it is the former. In which case there is nothing special about ScalarFunction.function_reference that prevents it from being 0.
There was a problem hiding this comment.
I can confirm that the intent of this is:
- It is required that this field be set.
- 0 is a valid anchor / reference
benbellick
force-pushed
the
benbellick/consistent-0-anchor
branch
2 times, most recently
from
0c0da1c to
098e7f9
Compare
benbellick
force-pushed
the
benbellick/consistent-0-anchor
branch
from
098e7f9 to
f32460f
Compare
|
|
||
| // Note: type_variation_reference fields within Type messages reference a | ||
| // type_variation_anchor defined in the plan's extension declarations. The value | ||
| // 0 represents the system-preferred variation and is a valid reference value. |
There was a problem hiding this comment.
Wasn't sure where a good place to put this was considering type_variation_reference shows up a ton of times.
There was a problem hiding this comment.
For type variation anchor defined in the extension, it is better to not use 0 because it is reserved IMO. Perhaps, we can document this in the type extension proto message as well as type variation documentation?
benbellick
requested review from
EpsilonPrime,
cpcloud,
jacques-n,
vbarua,
westonpace and
yongchul
as code owners
|
Given that the proposed documentation changes say "non-negative" would it make sense to always say I do remember the substrait-validator having a check for the 0 anchors reporting a warning. Should probably be also updated at some point in time. |
|
I can provide some guidance on why the non-zero language exists for references. Effectively, if a The stance of the project has generally been What do you think about including the explicit guidance to avoid zero values? |
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Thanks for updating this. Left on minor comment, but otherwise looks good to me.
|
Should we also update tutorials for URN anchors to start from 0? Edit: I just reread this to understand:
I guess the idea is that anchors are unique, unordered values and they're all valid as long as they're unique (within a plan) at any given time? |
|
|
||
| // Note: type_variation_reference fields within Type messages reference a | ||
| // type_variation_anchor defined in the plan's extension declarations. The value | ||
| // 0 represents the system-preferred variation and is a valid reference value. |
There was a problem hiding this comment.
For type variation anchor defined in the extension, it is better to not use 0 because it is reserved IMO. Perhaps, we can document this in the type extension proto message as well as type variation documentation?
| // A surrogate key used in the context of a single plan to reference a | ||
| // specific type variation | ||
| // specific type variation. | ||
| // 0 is a valid anchor/reference, but prefer non-zero values for ergonomics. |
There was a problem hiding this comment.
shall we be explicit about using non-zero here?
There are lots of places where the validity of anchor/reference 0 is not explicitly mentioned. This PR adds explicitly clarification in the proto comments and the site docs to clarify that anchor/reference 0 is valid.
Closes #899