[SPARK-15365] [SQL]: When table size statistics are not available from metastore, we should fallback to HDFS #13150
Conversation
There was a problem hiding this comment.
(I remember I was told that chaining getOrElse is not preferred because it is confusing)
There was a problem hiding this comment.
What alternative was chosen as more readable/less confusing?
There was a problem hiding this comment.
yea there is nothing wrong with getOrElse, but chaining a lot of them together (also with option filter) can get really confusing. While you are at this, it'd be better to fix this.
Expanding it to be longer (and maybe use some imperative style code) could make it less confusing.
There was a problem hiding this comment.
@rxin I have replaced if with conventional if/elseif.
|
@Parth-Brahmbhatt this looks pretty good. However, given that hitting the underlying filesystem directly can incur a lot of latency (especially in case of S3), can you please conf protect this change (with a comment about the potential performance issues)? Additionally, perhaps it might be nice to set the conf to false by default to prevent silent regressions for existing queries (especially if we're targeting this for 2.0). |
|
@sameeragarwal Added config option. @rxin can you take a look one more time? |
…m metastore, fall back to HDFS.
…of stats or not. Default is false.
|
@sameeragarwal @rxin FYI, Github currently has some latency issues so you probably can't see the updates. |
Parth-Brahmbhatt
force-pushed
the
SPARK-15365
branch
from
04e013d to
f5d5dde
Compare
|
|
||
| val ENABLE_FALL_BACK_TO_HDFS_FOR_STATS = | ||
| SQLConfigBuilder("spark.sql.enableFallBackToHdfsForStats") | ||
| .doc("If the table statistics are not available from table metadata enable fall back to hdfs" + |
There was a problem hiding this comment.
nit: missing period after hdfs
|
LGTM |
|
jenkins test this please |
|
Test build #59215 has finished for PR 13150 at commit
|
| .getOrElse(sparkSession.sessionState.conf.defaultSizeInBytes))) | ||
| // if the size is still less than zero, we try to get the file size from HDFS. | ||
| // given this is only needed for optimization, if the HDFS call fails we return the default. | ||
| if (Option(totalSize).map(_.toLong).getOrElse(0L) > 0) { |
There was a problem hiding this comment.
can we write something like this to make it easier to read?
if (totalSize != null && totalSize.toLong > 0L) {
totalSize.toLong
} else if (rawDataSize != null && rawDataSize.toLong > 0) {
rawDataSize.toLong
} else if (sparkSession.sessionState.conf.fallBackToHdfsForStatsEnabled) {
...
} else {
sparkSession.sessionState.conf.defaultSizeInBytes
}
|
This looks good. Just two minor nits. If you can fix those that would be great. Also - would it be possible to add a test case? |
|
@rxin added a test case. |
|
Great - thanks. Jenkins, test this please. |
|
|
||
| sql( | ||
| s"""CREATE EXTERNAL TABLE csv_table(page_id INT, impressions INT) | ||
| ROW FORMAT SERDE 'org.apache.hadoop.hive.serde2.OpenCSVSerde' |
There was a problem hiding this comment.
need to indent this properly. I can fix this when I merge if Jenkins passes.
|
Test build #3017 has finished for PR 13150 at commit
|
|
Merging in master/2.0. |
…metastore, we should fallback to HDFS ## What changes were proposed in this pull request? Currently if a table is used in join operation we rely on Metastore returned size to calculate if we can convert the operation to Broadcast join. This optimization only kicks in for table's that have the statistics available in metastore. Hive generally rolls over to HDFS if the statistics are not available directly from metastore and this seems like a reasonable choice to adopt given the optimization benefit of using broadcast joins. ## How was this patch tested? I have executed queries locally to test. Author: Parth Brahmbhatt <[email protected]> Closes #13150 from Parth-Brahmbhatt/SPARK-15365. (cherry picked from commit 4acabab) Signed-off-by: Reynold Xin <[email protected]>
|
@Parth-Brahmbhatt you should add the email address you used in your commit to your github profile, so the commit is associated with your account. Thanks. |
|
@rxin Thanks for taking the time to review and merging the patch. I have added the Email to my profile. |
5 participants
What changes were proposed in this pull request?
Currently if a table is used in join operation we rely on Metastore returned size to calculate if we can convert the operation to Broadcast join. This optimization only kicks in for table's that have the statistics available in metastore. Hive generally rolls over to HDFS if the statistics are not available directly from metastore and this seems like a reasonable choice to adopt given the optimization benefit of using broadcast joins.
How was this patch tested?
I have executed queries locally to test.