Skip to content

ARROW-11319: [Rust] [DataFusion] Improve test comparisons to record batch, remove test::format_batch - #9264

Closed
alamb wants to merge 3 commits into
apache:masterfrom
alamb:remove_test_format_batch
Closed

ARROW-11319: [Rust] [DataFusion] Improve test comparisons to record batch, remove test::format_batch#9264
alamb wants to merge 3 commits into
apache:masterfrom
alamb:remove_test_format_batch

Conversation

@alamb

@alambalamb commented Jan 19, 2021

Copy link
Copy Markdown
Contributor

The test::format_batch function does not have wide range of type support (e.g. it doesn't support dictionaries) and its output makes tests hard to read / update, in my opinion. This PR consolidates the datafusion tests to use arrow::util::pretty::pretty_format_batches both to reduce code duplication as well as increase type support

This PR removes the test::format_batch(&batch); function and replaces it with arrow::util::pretty::pretty_format_batches and some macros. It has no code changes.

This change the following benefits:

  1. Better type support (I immediately can compare RecordBatches with Dictionary types in tests without having to update format_batch and ARROW-11289: [Rust][DataFusion] Implement GROUP BY support for Dictionary Encoded columns #9233 gets simpler)
  2. Better readability and error reporting (at least I find the code and diffs easier to understand)
  3. Easier test update / review: it is easier to update the diffs (you can copy/paste the test output into the source code) and to review them

This is a variant of a strategy that I been using with success in IOx source link and I wanted to contribute it back.

An example failure with this PR:

---- physical_plan::hash_join::tests::join_left_one stdout ----
thread 'physical_plan::hash_join::tests::join_left_one' panicked at 'assertion failed: `(left == right)`
left: `["+----+----+----+----+", "| a1 | b2 | c1 | c2 |", "+----+----+----+----+", "| 1 | 1 | 7 | 70 |", "| 2 | 2 | 8 | 80 |", "| 2 | 2 | 9 | 80 |", "+----+----+----+----+"]`,
right: `["+----+----+----+----+----+", "| a1 | b1 | c1 | a2 | c2 |", "+----+----+----+----+----+", "| 1 | 4 | 7 | 10 | 70 |", "| 2 | 5 | 8 | 20 | 80 |", "| 3 | 7 | 9 | | |", "+----+----+----+----+----+"]`:
expected:
[
"+----+----+----+----+",
"| a1 | b2 | c1 | c2 |",
"+----+----+----+----+",
"| 1 | 1 | 7 | 70 |",
"| 2 | 2 | 8 | 80 |",
"| 2 | 2 | 9 | 80 |",
"+----+----+----+----+",
]
actual:
[
"+----+----+----+----+----+",
"| a1 | b1 | c1 | a2 | c2 |",
"+----+----+----+----+----+",
"| 1 | 4 | 7 | 10 | 70 |",
"| 2 | 5 | 8 | 20 | 80 |",
"| 3 | 7 | 9 | | |",
"+----+----+----+----+----+",
]

You can copy/paste the output of actual directly into the test code for an update.

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

Comment threadrust/datafusion/src/test/mod.rs Outdated

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format batch is similar, but not the same, as pretty_print

The major difference is that it renders NULL values as "NULL" whereas the pretty printer renders them as an empty string ""

@alamb
alamb marked this pull request as ready for review January 20, 2021 13:11
@alamb

Copy link
Copy Markdown
ContributorAuthor

@jorgecarleitao@andygrove and @seddonm1 and @Dandandan -- what do you think of this approach to testing DataFusion output?

@andygroveandygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alamb I really like this and I like that it is now also testing the column names.

We may also want to consider moving the expected results out to files rather than having them in the code, especially the larger ones.

@DandandanDandandan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very useful @alamb 👍

@alamb

Copy link
Copy Markdown
ContributorAuthor

Rebasing to resolve a logical conflict (a new test that was added)

@alamb
alambforce-pushed the remove_test_format_batch branch from e904ae5 to 363e1f4CompareJanuary 23, 2021 11:22
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #9264 (363e1f4) into master (13e2134) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #9264 +/- ##
==========================================
- Coverage 81.87% 81.84% -0.03% 
==========================================
Files 215 215 Lines 53097 52949 -148 ==========================================
- Hits 43471 43336 -135 + Misses 9626 9613 -13 
Impacted FilesCoverage Δ
rust/datafusion/src/execution/context.rs88.25% <100.00%> (-1.11%)⬇️
...ust/datafusion/src/physical_plan/hash_aggregate.rs82.17% <100.00%> (-0.13%)⬇️
rust/datafusion/src/physical_plan/hash_join.rs83.40% <100.00%> (-0.48%)⬇️
rust/datafusion/src/test/mod.rs100.00% <100.00%> (+10.07%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13e2134...363e1f4. Read the comment docs.

@alambalamb changed the title ARROW-11319: [Rust] [DataFusion] Improve test comparisons to record batch, remove test::format_batchARROW-11319: [Rust] [DataFusion] Improve test comparisons to record batch, remove test::format_batchJan 23, 2021
@github-actions

Copy link
Copy Markdown

@alambalamb closed this in 67d0c2eJan 23, 2021

@jorgecarleitaojorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unable to review this in time, but it is a good idea. Thanks a lot @alamb .

I was unsure what happens when there is a string "NULL" on the result vs the slot being null, but that is something that we are already doing anyways with the current approach, so LGTM.

kszucs pushed a commit that referenced this pull request Jan 25, 2021
…atch, remove test::format_batch
The `test::format_batch` function does not have wide range of type support (e.g. it doesn't support dictionaries) and its output makes tests hard to read / update, in my opinion. This PR consolidates the datafusion tests to use `arrow::util::pretty::pretty_format_batches` both to reduce code duplication as well as increase type support
This PR removes the `test::format_batch(&batch);` function and replaces it with `arrow::util::pretty::pretty_format_batches` and some macros. It has no code changes.
This change the following benefits:
1. Better type support (I immediately can compare RecordBatches with `Dictionary` types in tests without having to update `format_batch` and #9233 gets simpler)
2. Better readability and error reporting (at least I find the code and diffs easier to understand)
3. Easier test update / review: it is easier to update the diffs (you can copy/paste the test output into the source code) and to review them
This is a variant of a strategy that I been using with success in IOx [source link](https://github.com/influxdata/influxdb_iox/blob/main/arrow_deps/src/test_util.rs#L15) and I wanted to contribute it back.
An example failure with this PR:
```
---- physical_plan::hash_join::tests::join_left_one stdout ----
thread 'physical_plan::hash_join::tests::join_left_one' panicked at 'assertion failed: `(left == right)`
left: `["+----+----+----+----+", "| a1 | b2 | c1 | c2 |", "+----+----+----+----+", "| 1 | 1 | 7 | 70 |", "| 2 | 2 | 8 | 80 |", "| 2 | 2 | 9 | 80 |", "+----+----+----+----+"]`,
right: `["+----+----+----+----+----+", "| a1 | b1 | c1 | a2 | c2 |", "+----+----+----+----+----+", "| 1 | 4 | 7 | 10 | 70 |", "| 2 | 5 | 8 | 20 | 80 |", "| 3 | 7 | 9 | | |", "+----+----+----+----+----+"]`:
expected:
[
"+----+----+----+----+",
"| a1 | b2 | c1 | c2 |",
"+----+----+----+----+",
"| 1 | 1 | 7 | 70 |",
"| 2 | 2 | 8 | 80 |",
"| 2 | 2 | 9 | 80 |",
"+----+----+----+----+",
]
actual:
[
"+----+----+----+----+----+",
"| a1 | b1 | c1 | a2 | c2 |",
"+----+----+----+----+----+",
"| 1 | 4 | 7 | 10 | 70 |",
"| 2 | 5 | 8 | 20 | 80 |",
"| 3 | 7 | 9 | | |",
"+----+----+----+----+----+",
]
```
You can copy/paste the output of `actual` directly into the test code for an update.
Closes#9264 from alamb/remove_test_format_batch
Authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@alamb@codecov-io@Dandandan@andygrove@jorgecarleitao