-
Notifications
You must be signed in to change notification settings - Fork 489
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
Add support for a paste operator in join.as() prefix specifications. #444
Add support for a paste operator in join.as() prefix specifications. #444
Conversation
The tests here demonstrate the use of the new as() implementation. All of these are now valid constructions:
The following is an example of a join that causes a runtime error because of duplicate names in the output stream.
|
385bf32
to
bac2dbf
Compare
…om join prefix names. Signed-off-by: Jon Seymour <[email protected]>
…Name. Signed-off-by: Jon Seymour <[email protected]>
…d duplicate prefixes Signed-off-by: Jon Seymour <[email protected]>
bac2dbf
to
63673e0
Compare
…d duplicate prefixes Signed-off-by: Jon Seymour <[email protected]>
I wonder if there is really a good reason to prevent "." appearing in a specified prefix? |
prefix1: 'abc' field1 'd' resulting fields using Any delimiter is needed to resolve issues where one of the prefixes is a prefix of the other. As long as you disallow the delimiter in the prefix conflicts are impossible. This case is obviously not likely but I'd rather just prevent it. |
f089852
to
d41c867
Compare
…om join prefix names. Signed-off-by: Jon Seymour <[email protected]>
…Name. Signed-off-by: Jon Seymour <[email protected]>
…d duplicate prefixes Signed-off-by: Jon Seymour <[email protected]>
…om join prefix names. Signed-off-by: Jon Seymour <[email protected]>
…Name. Signed-off-by: Jon Seymour <[email protected]>
…d duplicate prefixes Signed-off-by: Jon Seymour <[email protected]>
…te operator. Signed-off-by: Jon Seymour <[email protected]>
d41c867
to
7bfdcc1
Compare
@nathanielc I've rebased this change onto master. Ready for your review. |
} | ||
n := prefix + k | ||
if claim, ok := js.claims[n]; ok && claim != i { | ||
panic(fmt.Errorf("field %s of input %d conflicts with input %d", k, i, claim)) |
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.
Could we remove the panic here and return an error instead?
The calling functions JoinInto*
can then log the error and return models.Point/Batch{},false
the join will be skipped.
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.
I made use of panic because it complicates every caller of outName which would otherwise have to check for an error.
update: I had a quick look at removing the panic - it really does make things quite messy. However, if you really want me to do that, let me know and I will push such a change
@jonseymour I have been thinking about the user experience here and would like some feedback: First, to be clear the entire purpose of the To keep it simple I see two ways of going about the
There may be some middle ground but its not clear to me what that would be. I think if the ability to have full control over the field name is something we want then we go with #2. Based on your experience and others I believe we do need the control. Also by adding runtime checking for name conflicts we can remove the requirement to have an Thoughts? |
Yes, I think we do need to give the user more control and one of the implications of this change is that .as() becomes optional unless there is a conflict to resolve. |
Closing in favor of a simpler solution #698 The |
Per the discussion in #435, the join.as() method now:
An output column derived from field k of input stream i is now named as follows:
An error is detected at runtime if the output of a join node constructed using these rules would result in points with duplicate field names.
All existing, valid Tick scripts should operate per the current behaviour. Some Tick scripts that were invalid are now valid.