Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix size_of_scalar test #2531

Merged
merged 3 commits into from
May 16, 2022
Merged

Fix size_of_scalar test #2531

merged 3 commits into from
May 16, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented May 13, 2022

Which issue does this PR close?

N/A

Rationale for this change

This test is important. It verifies that the memory use of code like GroupByHash is not changed. Quoting:

        // Since ScalarValues are used in a non trivial number of places,
        // making it larger means significant more memory consumption
        // per distinct value.

It turns out that the way the cfgs were setup the test never was invoked.

The change seems to have come in via #1455 from @maxburke

I found this while reviewing #2523

What changes are included in this PR?

Fix test so it is always invoked

Are there any user-facing changes?

no

@alamb alamb marked this pull request as ready for review May 13, 2022 18:26
@github-actions github-actions bot added the datafusion Changes in the datafusion crate label May 13, 2022
@@ -377,7 +377,7 @@ mod tests {
#[cfg(target_arch = "aarch64")]
assert_eq!(std::mem::size_of::<ScalarValue>(), 64);

#[cfg(target_arch = "amd64")]
#[cfg(not(target_arch = "aarch64"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this formulation as it ensures that one of these assert_eq! calls will always be invoked, regardless of architecture

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also write something like

let expected = match cfg!(target_arch == "aarch64") {
  true => 64,
  false => 48,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        let expected = match cfg!(target_arch == "aarch64") {
            true => 64,
            false => 48,
        };

resulted in

error: expected 1 cfg-pattern
   --> datafusion/core/src/scalar.rs:380:30
    |
380 |         let expected = match cfg!(target_arch == "aarch64") {
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `cfg` (in Nightly builds, run with -Z macro-backtrace for more info)

for me locally

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, try removing the double equals...

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

A comment about why there is the architectural discrepancy might be nice

@@ -377,7 +377,7 @@ mod tests {
#[cfg(target_arch = "aarch64")]
assert_eq!(std::mem::size_of::<ScalarValue>(), 64);

#[cfg(target_arch = "amd64")]
#[cfg(not(target_arch = "aarch64"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also write something like

let expected = match cfg!(target_arch == "aarch64") {
  true => 64,
  false => 48,
};

@alamb alamb requested a review from tustvold May 16, 2022 12:04
@alamb alamb merged commit 2eb8797 into apache:master May 16, 2022
@alamb alamb deleted the alamb/fix_size_of_scalar branch May 16, 2022 17:33
korowa pushed a commit to korowa/arrow-datafusion that referenced this pull request May 18, 2022
* Fix size_of_scalar test

* add comments

* Update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants