[C++][Compute] Remove print statements from unit tests

[asfimport]

Unit tests should avoid unconditional print statements, preferring SCOPED_TRACE, ARROW_SCOPED_TRACE, or on-fail extra messages like ASSERT_EQ(1, 2) << "extra here";. There are some in the hash join unit tests

Reporter: Ben Kietzman / @bkietz
Assignee: Dhruv Vats / @dhruv9vats

PRs and other links:

• GitHub Pull Request #11663

_{Note: This issue was originally created as ARROW-14641. Please see the migration documentation for further details.}

[asfimport]

Dhruv Vats / @dhruv9vats:
Is it okay if I give this a shot?

If so, could you please point me in the direction I should start looking, as in, the specific files?

[asfimport]

Weston Pace / @westonpace:
Absolutely.  Do you have permission to assign the issue to yourself?  If so, go for it.  If not, let us know and we will get you that permission.

I think this was detected on cpp/src/arrow/compute/exec/hash_join_node_test.cc

Beyond that I don't have any list of files but all tests end in _test.cc and probably shouldn't have "cout" for the most part so that should be searchable.

[asfimport]

Dhruv Vats / @dhruv9vats:
This is my first time interacting with Apache Jira, so I don't think I have those permissions.

Also, from what I understand, on this line, the following code:

std::cout << join_type_name << " " << key_cmp_str << " ";
key_types.Print();
std::cout << " payload_l: ";
payload_types[0].Print();
std::cout << " payload_r: ";
payload_types[1].Print();
std::cout << " num_rows_l = " << num_rows_l << " num_rows_r = " << num_rows_r 
          << " batch size = " << batch_size
          << " parallel = " << (parallel ? "true" : "false");
std::cout << std::endl; 

Should turn into something like this:

ARROW_SCOPED_TRACE(join_type_name, " ",
                   key_cmp_str, " ",
                   key_types.Print());
ARROW_SCOPED_TRACE(" payload_l: ", payload_types[0].Print());
ARROW_SCOPED_TRACE(" payload_r: ", payload_types[1].Print());
ARROW_SCOPED_TRACE(" num_rows_l = ", num_rows_l,
                   " num_rows_r = ", num_rows_r,    
                   " batch size = ", batch_size,
                   " parallel = ", (parallel ? "true" : "false"));

right?

Also, the Print() method uses the SCOPED_TRACE macro internally too.

[asfimport]

Weston Pace / @westonpace:
I'd have to look closer at the test to tell for sure but the general idea is that ARROW_SCOPED_TRACE (which is just wrapping the google test concept of SCOPED_TRACE if you want to read up on it) should receive the minimum amount of information neccesary to identify the particular test case that is being run.  That way, if a test fails, instead of seeing "Expected 3 and got 7" you will see something like "When parallel = true, batch_size=7 we expected 3 and got 7".

At a glance, it looks like that might be more information that is necessary to include in SCOPED_TRACE.  For example, if "test case 7" always has 20 rows then you don't need to print both test case 7 AND 20 rows (just test case 7 is sufficient).  It's possible some of those print statements were included for utility while debugging and developing the unit tests.  That kind of print statement can be outright removed (and doesn't need to convert to scoped trace).

[asfimport]

Weston Pace / @westonpace:
Why don't you take your best guess and then put up a PR.  It will probably be easier for us to discuss specific statements further based on that PR.

[asfimport]

Weston Pace / @westonpace:
Issue resolved by pull request 11663
#11663