Sequences.withEffect() doesn't work well with LimitedSequence and probably some other kinds of Sequences

[leventov]

Sequences.limit(Sequences.withEffect(Sequences.simple(Arrays.asList(1, 2, 3)), ...), 3)

doesn't execute the effect. It looks like a bug to me. I don't see how it could be fixed with the current Yielder's API: isDone(), next(), and close(). Fixing the API would be a massive change, and I don't want to risk doing this change and rewrite very convoluted logic of Combining, Merging, Filtered, Limited Sequences.

FYI RxJava which has Flowable, the abstraction very similar to Sequence, uses Publisher/Subscriber model instead of "external" model like Yielder. Probably pub/sub allows to avoid such issues.

[leventov]

Specifically the test is

  @Test
  public void testLimitedOverEffectSequence() {
    for (int limit = 0; limit < 5; limit++) {
      Sequence<Integer> seq = Sequences.simple(Arrays.asList(1, 2, 3));
      final AtomicBoolean effectExecuted = new AtomicBoolean();
      Sequence<Integer> seqWithEffect = Sequences.withEffect(seq, new Runnable()
      {
        @Override
        public void run()
        {
          effectExecuted.set(true);
        }
      }, MoreExecutors.sameThreadExecutor());
      Sequence<Integer> limitedSeq = Sequences.limit(seqWithEffect, limit);
      Sequences.toList(limitedSeq, new ArrayList<Integer>());
      System.out.println("limit=" + limit + ": effect executed=" + effectExecuted.get());
    }
  }

[gianm]

I don't think this is a bug. The "effect" is only meant to execute if all results have been pulled out of the sequence. In this case, not all results have been pulled out of Sequences.simple(Arrays.asList(1, 2, 3)), so the effect shouldn't happen. This behavior makes it useful for cache population, where we wouldn't want to populate cache with a partial result set.

Sequences.withBaggage provides a way to make effects that always happen, so callers should use that if that's what they want.

[leventov]

@gianm why not all results are pulled? If you apply Sequences.toList() to this limited sequence, the result is [1, 2, 3], i. e. all results are pulled.

[gianm]

@leventov are you saying that the bug in this case is when the limit is 3? That makes sense to me. I ran your test and got:

limit=0: effect executed=false
limit=1: effect executed=false
limit=2: effect executed=false
limit=3: effect executed=false
limit=4: effect executed=true

To me those all look good except for the limit = 3 case.

[leventov]

@gianm yes, this corner case.

[gianm]

Got it, yeah, that one looks like a bug.

[github-actions]

This issue has been marked as stale due to 280 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If this issue is still
relevant, please simply write any comment. Even if closed, you can still revive the
issue at any time or discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

[github-actions]

This issue has been closed due to lack of activity. If you think that
is incorrect, or the issue requires additional review, you can revive the issue at
any time.