Skip to content

ARROW-10167: [Rust] [DataFusion] Support DictionaryArray in sql.rs tests, by using standard pretty printer - #8333

Closed
alamb wants to merge 1 commit into
apache:masterfrom
alamb:alamb/ARROW-10167-cleanup-sql-display
Closed

ARROW-10167: [Rust] [DataFusion] Support DictionaryArray in sql.rs tests, by using standard pretty printer#8333
alamb wants to merge 1 commit into
apache:masterfrom
alamb:alamb/ARROW-10167-cleanup-sql-display

Conversation

@alamb

@alambalamb commented Oct 3, 2020

Copy link
Copy Markdown
Contributor

This PR removes (most of) the special case pretty printing code in DataFusion's sql integration test, sql.rs in favor of the standard pretty printer in arrow::utils::pretty and in the process adds support for DictionaryArray printing as well as standardizing how tests are run and output is compared.

I am working on adding support for DictionaryArray in DataFusion (and thus want to add tests to sql.rs). This specific PR's changes are larger than strictly necessary, but I felt it made it easier to write new tests in sql.rs. However, if people prefer, I could instead add a special case for DictionaryArray in array_str and accomplish my goal with a much smaller PR

Note: I found that using Vec<Vec<String>> to encode expected results rather than a String retains the nice property that differences to expected output are shown reasonably in the test output. For example:

---- csv_query_cast_literal stdout ----
thread 'csv_query_cast_literal' panicked at 'assertion failed: `(left == right)`
left: `[["0.9294097332465232", "1.0"], ["0.3114712539863804", "1.0"]]`,
right: `[["0.9294097332465232", "1"], ["0.3114712539863804", "1"]]`', datafusion/tests/sql.rs:502:5

@github-actions

Copy link
Copy Markdown

@alambalamb changed the title ARROW-10159: [Rust] [DataFusion] Support DictionaryArray in sql.rs tests, by using standard pretty printerARROW-10167: [Rust] [DataFusion] Support DictionaryArray in sql.rs tests, by using standard pretty printerOct 4, 2020
@alamb
alambforce-pushed the alamb/ARROW-10167-cleanup-sql-display branch from 0b2dd5c to 9d22a54CompareOctober 4, 2020 11:00
@github-actions

Copy link
Copy Markdown

Comment threadrust/datafusion/tests/sql.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.

this is a pretty good example of the change in structure.

Comment threadrust/arrow/src/util/pretty.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.

Note that this type was missing in pretty.rs (I found it while converting the tests in sql.rs).

Comment threadrust/datafusion/tests/sql.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.

I did cheat somewhat here and punt on adding proper ListArray support to pretty.rs.

@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 this is a nice cleanup of the tests.

@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.

Really nice, @alamb . LGTM

@jorgecarleitao

Copy link
Copy Markdown
Member

@alamb we need a small rebase to merge.

@alamb
alambforce-pushed the alamb/ARROW-10167-cleanup-sql-display branch from da3aec1 to 177c6ceCompareOctober 6, 2020 09:37
@alamb

alamb commented Oct 6, 2020

Copy link
Copy Markdown
ContributorAuthor

@jorgecarleitao FYI rebased

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.

3 participants

@alamb@jorgecarleitao@andygrove