-
Notifications
You must be signed in to change notification settings - Fork 27
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
DRAFT #1213 added example of rowbuilder interface #1214
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for papaya-valkyrie-395400 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Looking good. |
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.
Looks good! Left a couple of comments, let me know what you think
} | ||
override def setKey(key: String): RowBuilder = { | ||
this.key = key | ||
this |
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.
Not sure if it applies here but would be worthwhile to think about:
It uses the same instance of the row builder so if somewhere we instantiate a row builder and share the same builder with other functions there is a chance that we might mutate the data in multiple places resulting in unexpected data changes and hard to find bugs.
For example:
def main() = {
...
val rowBuilder = InMemMapRowBuilder().setKey("key")
function1(rowBuilder)
function2(rowBuilder)
}
def function1(rowBuilder: RowBuilder, ...) = {
...
rowBuilder.setLong(col1, 50).asRow()
...
}
def function2(rowBuilder: RowBuilder) = {
...
// key will be "" here
rowBuilder.setString(col2, "ABC").asRow()
...
}
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, that is very true. Each caller should getetter own I think, but I think it's fair to assume different call sites shouldn't pass them across threads.
Will update and add notthreadsafe annotation
private val mutableMap = new mutable.HashMap[String, Any]() | ||
private var key: String = "" | ||
override def setLong(column: Column, v: Long): RowBuilder = { | ||
mutableMap.put(column.name, v) |
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.
For the future: might be able to check for type consistency by matching the type i.e. Long
with column.dataType
.
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, that was indeed my goal. So checking datatype of column matches call signature.
I will add that.
No description provided.