Spark: Add named constant for NO_ADVISORY_PARTITION_SIZE#15681
Spark: Add named constant for NO_ADVISORY_PARTITION_SIZE#15681singhpk234 merged 2 commits intoapache:mainfrom
NO_ADVISORY_PARTITION_SIZE#15681Conversation
| @@ -26,8 +26,10 @@ | |||
| /** A set of requirements such as distribution and ordering reported to Spark during writes. */ | |||
| public class SparkWriteRequirements { | |||
|
|
|||
| public static final long NO_ADVISORY_PARTITION_SIZE = 0; | |||
There was a problem hiding this comment.
Is there a reason this constant needs to be public? Could we make it private for now and only relax the visibility if a concrete use case comes up?
| @@ -54,6 +56,8 @@ public boolean hasOrdering() { | |||
|
|
|||
| public long advisoryPartitionSize() { | |||
| // Spark prohibits requesting a particular advisory partition size without distribution | |||
| return distribution instanceof UnspecifiedDistribution ? 0 : advisoryPartitionSize; | |||
| return distribution instanceof UnspecifiedDistribution | |||
There was a problem hiding this comment.
I liked how you moved this into SparkWriteRequirements constructor in the previous pr, I think it's weird that the class stores a value and then ignores it sometimes. We are currently basically saying "hey you can pass in a value, jk it does nothing because a different parameter was passed.
It would probably make sense to prohibit specifying advisory size if distribution is "unspecified" but I think that may break some things.
RussellSpitzer
left a comment
RussellSpitzer
left a comment
There was a problem hiding this comment.
Looks good to me, the only thing I think it's worth while to keep that constant private as suggested by @ebyhr. If you like you could change the assignment like in the previous pr.
|
Thanks for the feedback @RussellSpitzer & @ebyhr! I've addressed it in 0ff2adb |
|
Thanks for the change @jbewing ! Thank you for review @ebyhr @RussellSpitzer @huaxingao |
5 participants
This PR extracts a named constant for
NO_ADVISORY_PARTITION_SIZEinSparkWriteRequirements. I'm splitting this out of another PR which took another approach to a change and is making this change as a byproduct.Related PR: #15150