Unique tuple tags #4711
Unique tuple tags #4711
Conversation
| HashSet<String> inputNames = new HashSet<>(); | ||
| inputs.stream() | ||
| .forEach(i -> { | ||
| if (!inputNames.add(i.getTupleTag().getId())) { |
There was a problem hiding this comment.
I think I would just change this to:
assert(inputNames.add(i.getTupleTag().getId())
And not add IllegalArgumentException to these signatures
There was a problem hiding this comment.
oh ok I can do that. Can you please say why that is preferred?
There was a problem hiding this comment.
Sure! It's just a small enough edge case that I don't think it warrants changing a user-facing API signature for. Additionally it's the convention we're using elsewhere in scio-smb (check out at the constructors for all BucketMetadata implementations, which all use asserts)
There was a problem hiding this comment.
hmm ok I see the signature change is a good reason. But as far as I know the main difference between assert and throwing an exception is the ability to turn assert off in prod (it is a development feature). Isn't that why we use it in other places?
There was a problem hiding this comment.
in general it is easy to know where assert should be used and where it should not. e.g. when the input is set by user, you should NOT use assertion. In this case "the user" is the developer which chooses the name of the tuple tags? But that is at development time from their POV, while they are using our prod version of lib. confusing!
please let me know wyt :)
There was a problem hiding this comment.
also this is a runtime exception so no need to have it in the signature. Now the signature is unchanged
| @@ -110,12 +128,14 @@ private CoGbkWithSecondaryBuilder(Class<K1> primaryKeyClass, Class<K2> secondary | |||
| * Returns a new {@link CoGbkWithSecondary} with the given first sorted-bucket source in {@link | |||
| * Read}. | |||
| */ | |||
| public CoGbkWithSecondary<K1, K2> of(Read<?> read) { | |||
There was a problem hiding this comment.
Looks like this method got deleted?
There was a problem hiding this comment.
My IDE made a lot of white space changes, I made a bunch of mistakes fixing them back :)
farzad-sedghi
force-pushed
the
unique-tuple-tags
branch
from
aa81b54
to
3ae0da3
Compare
Codecov Report
@@ Coverage Diff @@
## main #4711 +/- ##
==========================================
- Coverage 60.94% 60.93% -0.02%
==========================================
Files 286 286
Lines 10467 10479 +12
Branches 750 755 +5
==========================================
+ Hits 6379 6385 +6
- Misses 4088 4094 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
farzad-sedghi
force-pushed
the
unique-tuple-tags
branch
from
3ae0da3
to
efeefcd
Compare
| @@ -69,7 +69,7 @@ final class SortedBucketScioContext(@transient private val self: ScioContext) ex | |||
| rhs: SortedBucketIO.Read[R], | |||
| targetParallelism: TargetParallelism = TargetParallelism.auto() | |||
| ): SCollection[(K, (L, R))] = { | |||
| val t = SortedBucketIO.read(keyClass).of(lhs).and(rhs).withTargetParallelism(targetParallelism) | |||
| val t = SortedBucketIO.read(keyClass).of(lhs, rhs).withTargetParallelism(targetParallelism) | |||
There was a problem hiding this comment.
Why did you move from approach with and? To easier catch all TupleTags in one method?
There was a problem hiding this comment.
that was the convention used in the generated smb multi join file. Also it causes a bit less object overhead, not that it matters much
farzad-sedghi
force-pushed
the
unique-tuple-tags
branch
3 times, most recently
from
132a5fb
to
4b31c87
Compare
farzad-sedghi
force-pushed
the
unique-tuple-tags
branch
from
4b31c87
to
2b1dcdb
Compare
farzad-sedghi
force-pushed
the
unique-tuple-tags
branch
3 times, most recently
from
1ce100a
to
ae8bb67
Compare
farzad-sedghi
force-pushed
the
unique-tuple-tags
branch
from
ae8bb67
to
85243d9
Compare
farzad-sedghi
force-pushed
the
unique-tuple-tags
branch
from
52380d3
to
acc4dd4
Compare
* check tuple tag id uniqueness for smb cogroup * test cases * update documentations
#4692