Skip to content

[SPARK-3011][SQL] _temporary directory should be filtered out by sqlContext.parquetFile #1959

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.

Sign up for GitHub

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

Closed
ghost wants to merge 5 commits into from
Closed

[SPARK-3011][SQL] _temporary directory should be filtered out by sqlContext.parquetFile #1959

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8fc32a
filter out tmp dir
josephsu Aug 13, 2014
c7e44f2
match syntax
josephsu Aug 14, 2014
40ea9bd
fix hadoop-0.23 compile error
josephsu Aug 15, 2014
8fe2398
add note to explain
josephsu Aug 15, 2014
be30793
remove .* and _* except _metadata
josephsu Aug 23, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/parquet/ParquetTypes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,9 +373,11 @@ private[parquet] object ParquetTypesConverter extends Logging {
}
ParquetRelation.enableLogForwarding()

// NOTE: Explicitly list "_temporary" because hadoop 0.23 removed the variable TEMP_DIR_NAME
// from FileOutputCommitter. Check MAPREDUCE-5229 for the detail.
val children = fs.listStatus(path).filterNot { status =>
val name = status.getPath.getName
name(0) == '.' || name == FileOutputCommitter.SUCCEEDED_FILE_NAME
name(0) == '.' || name == FileOutputCommitter.SUCCEEDED_FILE_NAME || name == "_temporary"
Copy link
Member

@srowen srowen Aug 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about ignoring any file starting with _ ? Hadoop (also) uses this convention, for things like the _SUCCESS file.

Copy link
Author

@ghost ghost Aug 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, that would ignore the metadata file "_metadata" as well.

Copy link
Contributor

@chutium chutium Aug 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should rethink about why we use filterNot here? simple filter works fine here, something like:

val children = fs.listStatus(path).filter { status =>
  val name = status.getPath.getName
  name == ParquetFileWriter.PARQUET_METADATA_FILE || (name(0) != '.' && name(0) != '_')
}

so we can ignore all of hidden/tmp files without _metadata

Copy link
Contributor

@liancheng liancheng Aug 23, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this. Just remove .* and _* except _metadata.

}

// NOTE (lian): Parquet "_metadata" file can be very slow if the file consists of lots of row
Copy link
Contributor

@chutium chutium Aug 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, a better solution for all of this could be:
no val children = fs.listStatus(path)... any more

then:

val metafile = fs.listStatus(path).find(_.getPath.getName == ParquetFileWriter.PARQUET_METADATA_FILE)
val datafile = fs.listStatus(path).find(isNotHiddenFile(_.getPath.getName))

this isNotHiddenFile simply check like this (name(0) != '.' && name(0) != '_')

then something like:

if datafile is not null
  return ParquetFileReader.readFooter(conf, datafile)
else
  return ParquetFileReader.readFooter(conf, metafile)

and moreover, @liancheng, after carefully reading following comments, finally i know what you mean "complete Parquet file on HDFS should be directory" #2044 (comment)

you mean the whole directory is "a single parquet file", and the files in it are "data"? but such a definition is really very very confusing... are you sure about this definition? i just googled, but found noting, only some like "Parquet files are self-describing so the schema is preserved"

so, since they are self-describing, in my mind, each "data-file" in a parquet file (a parquet-folder actually...) is also valid parquet-format-file, it should also be able to take as an input source for parquet reader like our Spark SQLContext...

Copy link
Contributor

@liancheng liancheng Aug 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listStatus(path) call can be expensive, especially when the path is an S3 URI. In� that case, an HTTP request is issued to get all the FileStatus objects. and it may take hundred of milliseconds to return (depending on how may HDFS files there are). That's why children is used.

And yea, I also find definition of "Parquet file" somewhat confusing, and even the official Parquet documentations doesn't provide a precise definition. IMO a part-* file is roughly equivalent to a row group of a Parquet file, but it also carries its own metadata and is thus self describing.

Copy link
Contributor

@chutium chutium Aug 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the info, yep, this method is a confusing point, maybe we can reference some other parquet reader implementation

Expand Down