Add more test cases on vector size #4240
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ray6080
force-pushed
the
vector-size
branch
from
036ea3b to
bbe6909
Compare
Benchmark ResultMaster commit hash:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4240 +/- ##
==========================================
- Coverage 86.55% 85.78% -0.77%
==========================================
Files 1397 1397
Lines 60487 60519 +32
Branches 7451 7453 +2
==========================================
- Hits 52355 51919 -436
- Misses 7963 8419 +456
- Partials 169 181 +12 ☔ View full report in Codecov by Sentry. |
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
1 task
royi-luo
force-pushed
the
vector-size
branch
3 times, most recently
from
a05e37f to
e595d1f
Compare
Benchmark ResultMaster commit hash:
|
royi-luo
reviewed
Feb 3, 2025
Comment on lines
+286
to
+289
| - name: Test with coverage vector capacity of 2 | ||
| run: | | ||
| make lcov VECTOR_CAPACITY_LOG2=1 | ||
|
|
There was a problem hiding this comment.
The compiling in the codecov jobs are much slower (I think we use different runners), I'll probably need to find a way to get this faster so this job isn't a bottleneck
Benchmark ResultMaster commit hash:
|
ray6080
commented
Feb 4, 2025
There was a problem hiding this comment.
I don't fully follow why we change this test case.
There was a problem hiding this comment.
I found that probably due to multiple threads running the lines that the warnings were reported on could change. I'll probably change this back and make it SKIP_VECTOR_SIZE_2
| @@ -41,7 +41,7 @@ void InsertDatasetByRow::init() { | |||
| auto query = stringFormat("call show_tables() where name='{}' return type;", tableName); | |||
| auto result = validateQuery(connection, query); | |||
| auto tableType = result->getNext()->getValue(0)->toString(); | |||
| query = stringFormat("call table_info('{}') return name, type order by 'property id';", | |||
| query = stringFormat("call table_info('{}') return name, type order by `property id`;", | |||
There was a problem hiding this comment.
hmm should we change column names to use underscore? Also what triggered the change in this PR?
There was a problem hiding this comment.
'property id' just returns the string property id. The other ticks are needed to actually get the property with the name.
There was a problem hiding this comment.
@andyfengHKU should take a look at changes here.
andyfengHKU
reviewed
Feb 4, 2025
| // If the overall probe result for the current key is empty we should emit a null tuple | ||
| // However if the current batch is empty but the overall probe result isn't we shouldn't emit an | ||
| // extra null tuple | ||
| if (probeState->matchedSelVector.getSelSize() == 0 && probeState->numTuplesForCurrentKey > 0) { |
There was a problem hiding this comment.
hmmmm I think this is still buggy in the following case.
For a given key, we probed 3 tuples, p1, p2 & p3. Suppose VECTOR size is 2.
In the first batch, p1, p2 there is nothing matched. We would output a null tuple.
Then in the second batch,
If p3 is matched, the we should NOT output the null tuple above. (this is still buggy!)
If p3 is not matched, we would generate a second null tuple. (your solution solves this)
Let's discuss with @ray6080 to see what's the right solution. I think we need to redesign the function call and left LEFT JOIN handle its own iteration logic.
There was a problem hiding this comment.
Opened #4861 and reverted the hash join probe changes, we'll address this in a future PR.
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
royi-luo
approved these changes
Feb 6, 2025
Benchmark ResultMaster commit hash:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Also update some tests + fix bugs caught by the new tests. Also add
SKIP_VECTOR_SIZE_2test token to skip tests that are expected to fail with vector size 2.Fixes: