Fix flaky TestPositionsAppender.testConsecutiveBuilds

[lukasz-stec]

Description

Since for nested types createRandomBlockForNestedType
chooses the lengths of the nested values randomly,
there is a chance that the chosen value length is
not big enough for the given nullRate.
To fix this, we cap the nested values nullRate, possibly to 0,
so it matches the nested position count.

Is this change a fix, improvement, new feature, refactoring, or other?

fix flaky test

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core query engine tests

How would you describe this change to a non-technical end user or system administrator?

fix flaky test

Related issues, pull requests, and links

• Fixes Flaky TestPositionsAppender.testConsecutiveBuilds #12875

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Fixes #12888

[raunaqmorarka]

Why is the variable named positionCount ? For map and array isn't offsets[positionCount] the total length of data in the Block ?

[lukasz-stec]

For map and array isn't offsets[positionCount] the total length of data in the Block ?

well yes, if by total length you mean position count of the data/value block

[raunaqmorarka]

Could you call it entryCount or valuesLength ? At least that's the terminology used in AbstractArrayBlock or AbstractMapBlock to distinguish it from positionCount.
Also consider passing down offsets and positionCount separately to this method if it's always meant to be used in context of Map and Row

[lukasz-stec]

This method is not specific to AbstractMapBlock or AbstractArrayBlock. It returns the nullRate that will match exactly the number of null positions for the given position count.

[sopel39]

Why can't we just fix chooseNullPositions to accept 0 nulls?

[lukasz-stec]

Why can't we just fix chooseNullPositions to accept 0 nulls?

It accepts 0 nulls already. it fails if the null rate is not 0 but the result would be empty. This is to guard against unintentional setup where non-zero nullRate generates 0 nulls.

[sopel39]

It accepts 0 nulls already. it fails if the null rate is not 0 but the result would be empty. This is to guard against unintentional setup where non-zero nullRate generates 0 nulls.

With capNullRate it seems we are purposefully mitigating that check, hence why to keep it in the first place?

[lukasz-stec]

It accepts 0 nulls already. it fails if the null rate is not 0 but the result would be empty. This is to guard against unintentional setup where non-zero nullRate generates 0 nulls.

With capNullRate it seems we are purposefully mitigating that check, hence why to keep it in the first place?

Yes, but only in the nested type case for the value block.
I'm also fine with dropping this check.

[sopel39]

Yes, but only in the nested type case for the value block.
I'm also fine with dropping this check.

Maybe a separate method without that check?

[lukasz-stec]

Yes, but only in the nested type case for the value block.
I'm also fine with dropping this check.

Maybe a separate method without that check?

do we need two? how would use this new method given there is a recursive call in createRandomBlockForNestedType?

[sopel39]

do we need two? how would use this new method given there is a recursive call in createRandomBlockForNestedType?

I think it's cleaner than adding a method with check and then creating another method that mitigates that check. You already have two methods

[lukasz-stec]

do we need two? how would use this new method given there is a recursive call in createRandomBlockForNestedType?

I think it's cleaner than adding a method with check and then creating another method that mitigates that check. You already have two methods

there is a recursive call in createRandomBlockForNestedType

[sopel39]

there is a recursive call in createRandomBlockForNestedType

Would it be possible to simply exclude test cases that generate 0 nulls?

[lukasz-stec]

there is a recursive call in createRandomBlockForNestedType

Would it be possible to simply exclude test cases that generate 0 nulls?

after offline discussion I changed this fix to make the BlockAssertions deterministic

[ksobolew]

Um, this actually uses a fresh Random instance per iteration. Each instance has the same seed, so each iteration will produce the same result. So this is kind of more deterministic than you wanted, I think :)

[lukasz-stec]

🤦 #13029 cc @sopel39