-
Notifications
You must be signed in to change notification settings - Fork 5
[SPARK-51815] Add Row
struct
#63
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
Conversation
a538a15
to
1ef9013
Compare
1ef9013
to
6d441c3
Compare
Could you review this |
@@ -217,7 +217,7 @@ public actor DataFrame: Sendable { | |||
values.append(str.asString(i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still stored the value as string? So we still cannot insert column values into Row in their types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes~ This is a part-one implementation to introduce Row
struct like the note in the PR description.
Note that Row is added to support general type fields, but this PR replaces the existing API's [String?] signature into Row-based signature. The detailed one-to-one mapping among other types will be handled later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thank you.
[Location,*] | ||
[Owner,*] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why the the second elements of the two items ["Location","file:/opt/spark/work-dir/spark-warehouse"],["Owner","185"] are *
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the Catalog
PR, the cleaning methods were added previously. While regenerating answer files Today, I used the cleaned strings because the cleaned one is better in the GitHub repo.
spark-connect-swift/Tests/SparkConnectTests/SQLTests.swift
Lines 36 to 50 in 21724f5
private func cleanUp(_ str: String) -> String { | |
return removeOwner(removeID(removeLocation(str))) | |
} | |
private func removeID(_ str: String) -> String { | |
return str.replacing(regexPlanId, with: "plan_id=").replacing(regexID, with: "#") | |
} | |
private func removeLocation(_ str: String) -> String { | |
return str.replacing(regexLocation, with: "*") | |
} | |
private func removeOwner(_ str: String) -> String { | |
return str.replacing(regexOwner, with: "*") | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thank you.
Thank you! Merged to main. |
What changes were proposed in this pull request?
This PR aims to add
Row
struct and use it.Why are the changes needed?
To make
DataFrame
APIs returnRow
instead of[String?]
inSwift
.Note that
Row
is added to support general type fields, but this PR replaces the existing API's[String?]
signature intoRow
-based signature. The detailed one-to-one mapping among other types will be handled later.Does this PR introduce any user-facing change?
Yes, but this is a change to the unreleased version.
How was this patch tested?
Pass the CIs.
Was this patch authored or co-authored using generative AI tooling?
No.