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

GH-42245: [Swift] Ensure map behavior is the same for all key types #42246

Merged
merged 1 commit into from
Jun 22, 2024

Conversation

abandy
Copy link
Contributor

@abandy abandy commented Jun 21, 2024

Rationale for this change

Behavior of decoding a map currently changes based on the key type (String or not String)

What changes are included in this PR?

Added method for handling map to ensure that all maps are decoded the same way.

Are these changes tested?

Yes

Comment on lines 129 to 130
let array: AnyArray = try self.getCol(self.singleRBCol)
return array.asAny(self.rbIndex) as! T // swiftlint:disable:this force_cast
Copy link
Member

Choose a reason for hiding this comment

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

Can we call doDecode<T>(_ col: Int) instead of adding a similar code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. My goal was to make it clear that the index for the single column was being handled a bit different but sending in the singleRBCol to the doDecode method works.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, defining doDecodeSingleValue() and isNullSingleValue() is OK. I just wanted to say that changing the implementation something like:

    func doDecodeSingleValue<T>() throws -> T? {
        return doDecode(self.singleRBCol)
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, I see! Thanks for clarifying! We do remove the overhead of another func call by removing the SingleValue methods. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

If there is a performance overhead, I'm OK with the current approach.

Comment on lines 144 to 145
let array: AnyArray = try self.getCol(self.singleRBCol)
return array.asAny(self.rbIndex) == nil
Copy link
Member

Choose a reason for hiding this comment

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

Can we call isNull(_ col: Int) instead?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 22, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jun 22, 2024
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou kou merged commit d28078d into apache:main Jun 22, 2024
11 checks passed
@kou kou removed the awaiting changes Awaiting changes label Jun 22, 2024
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jun 22, 2024
Copy link

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit d28078d.

There were 12 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to 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.

None yet

2 participants