Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ public static CaseInsensitiveStringMap empty() {
return new CaseInsensitiveStringMap(new HashMap<>(0));
}

private final Map<String, String> original;

private final Map<String, String> delegate;

public CaseInsensitiveStringMap(Map<String, String> originalMap) {
this.original = new HashMap<>(originalMap.size());
this.delegate = new HashMap<>(originalMap.size());
putAll(originalMap);
}
Expand Down Expand Up @@ -78,11 +81,13 @@ public String get(Object key) {

@Override
public String put(String key, String value) {
original.put(key, value);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The thing worries me most is the inconsistency between the case insensitive map and the original map. I think we should either fail or keep the latter entry if a -> 1, A -> 2 appears together.

One thing we can simplify is, CaseInsensitiveStringMap is read by data source and can be read-only. Then it can be easier to resolve conflicting entries at the beginning.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But the method put/remove/clear still need to be implemented...Do you mean that we can ignore original map in these method ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can just throw exception in these methods, and say this map is readonly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@cloud-fan This is a good solution!
@rdblue What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That works for me. It is always a good idea to avoid passing mutable state to plugin code.

return delegate.put(toLowerCase(key), value);
}

@Override
public String remove(Object key) {
original.remove(key);
return delegate.remove(toLowerCase(key));
}

Expand All @@ -95,6 +100,7 @@ public void putAll(Map<? extends String, ? extends String> m) {

@Override
public void clear() {
original.clear();
delegate.clear();
}

Expand Down Expand Up @@ -157,4 +163,11 @@ public double getDouble(String key, double defaultValue) {
String value = get(key);
return value == null ? defaultValue : Double.parseDouble(value);
}

/**
* Returns the original case-sensitive map.
*/
public Map<String, String> getOriginalMap() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about asCaseSensitiveMap? That is more clear that using "original" because that doesn't indicate why someone would call this method.

return original;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should return a read-only version of original. You can use Collections.unmodifiableMap.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.spark.sql.util

import java.util

import scala.collection.JavaConverters._

import org.apache.spark.SparkFunSuite
Expand Down Expand Up @@ -80,4 +82,30 @@ class CaseInsensitiveStringMapSuite extends SparkFunSuite {
options.getDouble("foo", 0.1d)
}
}

test("getOriginalMap") {
val originalMap = new util.HashMap[String, String] {
put("Foo", "Bar")
put("OFO", "ABR")
put("OoF", "bar")
}

val options = new CaseInsensitiveStringMap(originalMap)
assert(options.getOriginalMap.equals(originalMap))

val key = "Key"
val value = "value"
originalMap.put(key, value)
options.put(key, value)
originalMap.put(key, value)
assert(options.getOriginalMap.equals(originalMap))

val removedKey = "OFO"
originalMap.remove(removedKey)
options.remove(removedKey)
assert(options.getOriginalMap.equals(originalMap))

options.clear()
assert(options.getOriginalMap.isEmpty)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ abstract class FileTable(
extends Table with SupportsBatchRead with SupportsBatchWrite {

lazy val fileIndex: PartitioningAwareFileIndex = {
val scalaMap = options.asScala.toMap
val hadoopConf = sparkSession.sessionState.newHadoopConfWithOptions(scalaMap)
val hadoopConf = sparkSession.sessionState.newHadoopConfWithCaseInsensitiveOptions(options)
val rootPathsSpecified = DataSource.checkAndGlobPathIfNecessary(paths, hadoopConf,
checkEmptyGlobPath = true, checkFilesExist = true)
val fileStatusCache = FileStatusCache.getOrCreate(sparkSession)
val scalaMap = options.getOriginalMap.asScala.toMap
new InMemoryFileIndex(
sparkSession, rootPathsSpecified, scalaMap, userSpecifiedSchema, fileStatusCache)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,14 @@ abstract class FileWriteBuilder(options: CaseInsensitiveStringMap, paths: Seq[St
val sparkSession = SparkSession.active
validateInputs(sparkSession.sessionState.conf.caseSensitiveAnalysis)
val path = new Path(paths.head)
val optionsAsScala = options.asScala.toMap

val hadoopConf = sparkSession.sessionState.newHadoopConfWithOptions(optionsAsScala)
val hadoopConf = sparkSession.sessionState.newHadoopConfWithCaseInsensitiveOptions(options)
val job = getJobInstance(hadoopConf, path)
val committer = FileCommitProtocol.instantiate(
sparkSession.sessionState.conf.fileCommitProtocolClass,
jobId = java.util.UUID.randomUUID().toString,
outputPath = paths.head)
lazy val description =
createWriteJobDescription(sparkSession, hadoopConf, job, paths.head, optionsAsScala)
createWriteJobDescription(sparkSession, hadoopConf, job, paths.head, options.asScala.toMap)

val fs = path.getFileSystem(hadoopConf)
mode match {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ case class OrcScanBuilder(
dataSchema: StructType,
options: CaseInsensitiveStringMap)
extends FileScanBuilder(schema) with SupportsPushDownFilters {
lazy val hadoopConf = sparkSession.sessionState.newHadoopConfWithOptions(options.asScala.toMap)
lazy val hadoopConf = sparkSession.sessionState.newHadoopConfWithCaseInsensitiveOptions(options)

override def build(): Scan = {
OrcScan(sparkSession, hadoopConf, fileIndex, dataSchema, readSchema, options)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package org.apache.spark.sql.internal

import java.io.File

import scala.collection.JavaConverters._

import org.apache.hadoop.conf.Configuration
import org.apache.hadoop.fs.Path

Expand All @@ -32,7 +34,7 @@ import org.apache.spark.sql.catalyst.parser.ParserInterface
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.execution._
import org.apache.spark.sql.streaming.StreamingQueryManager
import org.apache.spark.sql.util.{ExecutionListenerManager, QueryExecutionListener}
import org.apache.spark.sql.util.{CaseInsensitiveStringMap, ExecutionListenerManager, QueryExecutionListener}
Comment thread
gengliangwang marked this conversation as resolved.
Outdated

/**
* A class that holds all session-specific state in a given [[SparkSession]].
Expand Down Expand Up @@ -96,6 +98,11 @@ private[sql] class SessionState(
hadoopConf
}

def newHadoopConfWithCaseInsensitiveOptions(options: CaseInsensitiveStringMap): Configuration = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should pollute SessionState with the case insensitive map stuff. Can we inline this method?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Otherwise, developers might not be aware of using .getOriginalMap if they want to create Hadoop configuration from CaseInsensitiveStringMap.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then we should document it in CaseInsensitiveMap. data source developers can't access SessionState

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with @cloud-fan, I'd rather use newHadoopConfWithOptions and not add a new method here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should pollute SessionState with the case insensitive map stuff. Can we inline this method?

// Hadoop configurations are case sensitive.
newHadoopConfWithOptions(options.getOriginalMap.asScala.toMap)
}

/**
* Get an identical copy of the `SessionState` and associate it with the given `SparkSession`
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package org.apache.spark.sql.execution.datasources.orc

import java.io.File

import org.apache.hadoop.fs.{Path, PathFilter}

import org.apache.spark.SparkConf
import org.apache.spark.sql._
import org.apache.spark.sql.internal.SQLConf
Expand All @@ -30,6 +32,10 @@ case class OrcParData(intField: Int, stringField: String)
// The data that also includes the partitioning key
case class OrcParDataWithKey(intField: Int, pi: Int, stringField: String, ps: String)

class TestFileFilter extends PathFilter {
override def accept(path: Path): Boolean = path.getParent.getName != "p=2"
}

abstract class OrcPartitionDiscoveryTest extends OrcTest {
val defaultPartitionName = "__HIVE_DEFAULT_PARTITION__"

Expand Down Expand Up @@ -226,6 +232,23 @@ abstract class OrcPartitionDiscoveryTest extends OrcTest {
}
}
}

test("SPARK-27162: handle pathfilter configuration correctly") {
withTempPath { dir =>
val path = dir.getCanonicalPath

val df = spark.range(2)
df.write.orc(path + "/p=1")
df.write.orc(path + "/p=2")
assert(spark.read.orc(path).count() === 4)

val extraOptions = Map(
"mapred.input.pathFilter.class" -> classOf[TestFileFilter].getName,
"mapreduce.input.pathFilter.class" -> classOf[TestFileFilter].getName
)
assert(spark.read.options(extraOptions).orc(path).count() === 2)
}
}
}

class OrcPartitionDiscoverySuite extends OrcPartitionDiscoveryTest with SharedSQLContext
Expand Down