You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by swapnilushinde <gi...@git.apache.org> on 2018/07/17 04:36:01 UTC
[GitHub] spark pull request #21787: [SPARK-24568] Code refactoring for DataType equal...
GitHub user swapnilushinde opened a pull request:
https://github.com/apache/spark/pull/21787
[SPARK-24568] Code refactoring for DataType equalsXXX methods
## What changes were proposed in this pull request?
Currently, DataType equals* methods has lot of code duplication. This PR adds -
1. Helper function that removes code duplication and makes it easier to understand.
2. It makes adding new variations of equals* methods easy by simply choosing right parameter combinations of helper function.
## Current functionality -
DataTypes are matched on three factors - (fieldName, dataType, nullability)
Current APIs
> 1. equalsIgnoreNullability - (fieldNameCheck: Yes, dataType: Yes, nullability: No)
> 2. equalsIgnoreCompatibleNullability - (fieldNameCheck: Yes, dataType: Yes, nullability: Compatible)
> 3. equalsIgnoreCaseAndNullability - (fieldNameCheck: Yes(case insensitive), dataType: Yes, nullability: No)
> 4. equalsStructurally - (fieldNameCheck: No, dataType: Yes, nullability: Yes/No)
Same API contracts are maintained but just new helper functions equalsDataTypes, isSameFieldName, isSameNullability added to give more flexibility & code centralization.
Additional private vals are added to set behavior of helper functions.
```
private val NoNameCheck = 0
private val CaseSensitiveNameCheck = 1
private val CaseInsensitiveNameCheck = 2
private val NoNullabilityCheck = 0
private val NullabilityCheck = 1
private val CompatibleNullabilityCheck = 2
```
Any combination of above variables can be generated in future to add new equals* APIs. For instance,
> equalsIgnoreCaseCompatibleNullability
can easily be generated by calling -
> equalsDataTypes(left, right, CaseInsensitiveNameCheck, CompatibleNullabilityCheck)
New scalatests added to test missing API unit testing as well.
## How was this patch tested?
Code was tested with existing scalatests. New scalatests added for more robust testing of this functionality.
All tests were ran locally to make sure it doesn't affect elsewhere.
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/swapnilushinde/spark SPARK-24568
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21787.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21787
----
commit 73ff0324278f98080e69a6de84c45d2fd4581b1e
Author: Shinde, Swapnil <ss...@...>
Date: 2018-07-17T03:59:21Z
[SPARK-24568] Code refactoring for DataType equalsXXX methods
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21787: [SPARK-24568] Code refactoring for DataType equalsXXX me...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21787
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21787: [SPARK-24568] Code refactoring for DataType equalsXXX me...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21787
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21787: [SPARK-24568] Code refactoring for DataType equalsXXX me...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21787
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21787: [SPARK-24568] Code refactoring for DataType equalsXXX me...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21787
But the current code looks less readable and adding more lines. I would rather leave this as was.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21787: [SPARK-24568] Code refactoring for DataType equalsXXX me...
Posted by swapnilushinde <gi...@git.apache.org>.
Github user swapnilushinde commented on the issue:
https://github.com/apache/spark/pull/21787
true. Currently we have just 3 variations of comparing two datatypes for equality. Adding even one more equality function would easily cause writing same repetitive code which would negate observed increase in code lines.
Comparing two datasets' with different criteria for datatype equality is very handy for unit testing purposes too.
Can you please let me know what do you find unreadable/complex in new code so I can try to simplify it if possible?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21787: [SPARK-24568] Code refactoring for DataType equal...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21787
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21787: [SPARK-24568] Code refactoring for DataType equalsXXX me...
Posted by swapnilushinde <gi...@git.apache.org>.
Github user swapnilushinde commented on the issue:
https://github.com/apache/spark/pull/21787
@HyukjinKwon I removed old commented code in new comment.
Old code was repeating same recursive datatype checks again & again in every equals* function. This commit abstracts that recursive equality checks and gives flexibility to easily add any combination of equals* method in future.
Equals* functions are just one line with this commit.
Please let me know if I need to change anything to make code changes more readable.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21787: [SPARK-24568] Code refactoring for DataType equalsXXX me...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21787
Constant variables and squashing the logic into one function look not worth enough and overkill. Less duplication is good of course but it doesn't look worth enough for both. I would focus on more important stuff. -1 from me.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org