text:
[SPARK-32621][SQL] 'path' option can cause issues while inferring schema in CSV/JSON datasources

What changes were proposed in this pull request?

When CSV/JSON datasources infer schema (e.g, def inferSchema(files: Seq[FileStatus]), they use the files along with the original options. files in inferSchema could have been deduced from the "path" option if the option was present, so this can cause issues (e.g., reading more data, listing the path again) since the "path" option is added to the files.
Why are the changes needed?

The current behavior can cause the following issue:
class TestFileFilter extends PathFilter {
  override def accept(path: Path): Boolean = path.getParent.getName != "p=2"
}

val path = "/tmp"
val df = spark.range(2)
df.write.json(path + "/p=1")
df.write.json(path + "/p=2")

val extraOptions = Map(
  "mapred.input.pathFilter.class" -> classOf[TestFileFilter].getName,
  "mapreduce.input.pathFilter.class" -> classOf[TestFileFilter].getName
)

// This works fine.
assert(spark.read.options(extraOptions).json(path).count == 2)

// The following with "path" option fails with the following:
// assertion failed: Conflicting directory structures detected. Suspicious paths
//	file:/tmp
//	file:/tmp/p=1
assert(spark.read.options(extraOptions).format("json").option("path", path).load.count() === 2)

Does this PR introduce any user-facing change?

Yes, the above failure doesn't happen and you get the consistent experience when you use spark.read.csv(path) or spark.read.format("csv").option("path", path).load.
How was this patch tested?

Updated existing tests.

Test build #127469 has started for PR 29437 at commit 23f2efe.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32090/
Test PASSed.

Test build #127469 has finished for PR 29437 at commit 23f2efe.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127469/
Test PASSed.

Test build #127481 has started for PR 29437 at commit cfc9035.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32102/
Test PASSed.

Test build #127481 has finished for PR 29437 at commit cfc9035.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127481/
Test PASSed.

cc @cloud-fan @viirya

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32137/
Test PASSed.

Test build #127514 has started for PR 29437 at commit b7f4ff6.

Test build #127514 has finished for PR 29437 at commit b7f4ff6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127514/
Test PASSed.

Test build #127594 has started for PR 29437 at commit 431a55d.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32217/
Test PASSed.

Test build #127595 has started for PR 29437 at commit 15fd313.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32218/
Test PASSed.

Test build #127595 has finished for PR 29437 at commit 15fd313.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127595/
Test FAILed.

Test build #127597 has started for PR 29437 at commit 6d5b2d3.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32220/
Test PASSed.

Test build #127594 has finished for PR 29437 at commit 431a55d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127594/
Test PASSed.

Test build #127597 has finished for PR 29437 at commit 6d5b2d3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127597/
Test PASSed.

Test build #127624 has started for PR 29437 at commit 6f147ee.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32248/
Test PASSed.

Test build #127624 has finished for PR 29437 at commit 6f147ee.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127624/
Test FAILed.

retest this please

retest this please

Test build #127630 has started for PR 29437 at commit 6f147ee.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/32254/
Test PASSed.

Test build #127630 has finished for PR 29437 at commit 6f147ee.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/127630/
Test PASSed.

Merged build finished. Test PASSed.

thanks, merging to master!

@imback82 can you open a new PR for 3.0?

@imback82 can you open a new PR for 3.0?

Yes, will do.
Thanks for the review @HyukjinKwon @cloud-fan @viirya @gengliangwang!

authors: AmplabJenkins SparkQA cloud-fan imback82


text:
[SPARK-39325][CORE]Improve MapOutputTracker convertMapStatuses performance

What changes were proposed in this pull request?
Optimize MapOutputTracker.convertMapStatuses() method.
Why are the changes needed?
MapOutputTracker.convertMapStatuses() will be very slow if there are tens of thousands MapStatuses and MergeStatuses.
Benchmark code:
  val benchmark = new Benchmark("MapStatuses Convert", 1, output = output)

    val blockManagerNumber = 1000
    val mapNumber = 50000
    val shufflePartitions = 10000

    val shuffleId: Int = 0
    // First reduce task will fetch map data from startPartition to endPartition
    val startPartition = 0
    val startMapIndex = 0
    val endMapIndex = mapNumber
    val blockManagers = Array.tabulate(blockManagerNumber) { i =>
      BlockManagerId("a", "host" + i, 7337)
    }
    val mapStatuses: Array[MapStatus] = Array.tabulate(mapNumber) { mapTaskId =>
      HighlyCompressedMapStatus(
        blockManagers(mapTaskId % blockManagerNumber),
        Array.tabulate(shufflePartitions)(i => if (i % 50 == 0) 1 else 0),
        mapTaskId)
    }
    val bitmap = new RoaringBitmap()
    Range(0, 4000).foreach(bitmap.add(_))
    val mergeStatuses = Array.tabulate(shufflePartitions) { part =>
      MergeStatus(blockManagers(part % blockManagerNumber), shuffleId, bitmap, 100)
    }

    Array(499, 999, 1499).foreach { endPartition =>
      benchmark.addCase(
        s"Num Maps: $mapNumber Fetch partitions:${endPartition - startPartition + 1}",
        numIters) { _ =>
        MapOutputTracker.convertMapStatuses(
          shuffleId,
          startPartition,
          endPartition,
          mapStatuses,
          startMapIndex,
          endMapIndex,
          Some(mergeStatuses))
      }
    }

Before this PR
================================================================================================
MapStatuses Convert Benchmark
================================================================================================

Java HotSpot(TM) 64-Bit Server VM 1.8.0_281-b09 on Mac OS X 10.15.7
Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
MapStatuses Convert:                      Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
Num Maps: 50000 Fetch partitions:500               3393           3483          96          0.0  3393439257.0       1.0X
Num Maps: 50000 Fetch partitions:1000              6640           6772         121          0.0  6639654832.0       0.5X
Num Maps: 50000 Fetch partitions:1500             10035          10143         108          0.0 10035100069.0       0.3X

After this PR
================================================================================================
MapStatuses Convert Benchmark
================================================================================================

Java HotSpot(TM) 64-Bit Server VM 1.8.0_281-b09 on Mac OS X 10.15.7
Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
MapStatuses Convert:                      Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
Num Maps: 50000 Fetch partitions:500                667            679          15          0.0   666562302.0       1.0X
Num Maps: 50000 Fetch partitions:1000              1285           1397         115          0.0  1284808865.0       0.5X
Num Maps: 50000 Fetch partitions:1500              2045           2068          32          0.0  2044951906.0       0.3X

Does this PR introduce any user-facing change?
No
How was this patch tested?
Exists UTs.

Can one of the admins verify this patch?

cc @mridulm @Victsm

I will take a look at this later this week.
Looks like a good improvement to add, thanks for the PR !

+CC @Ngone51 as well, since you had reviewed the original change.

Thank you all. Merged to master.

@dongjoon-hyun I don't think this is a regression since all these changes are for push-based shuffles.

source: gharchive/pull-request

authors: AmplabJenkins mridulm wangyum wankunde

repo: apache/spark

license_type: permissive

license_source: github-api


text:
[SPARK-40480][SHUFFLE] Remove push-based shuffle data after query finished

What changes were proposed in this pull request?
Cleanup merged shuffle data files after query finished.
The main changes are:

After push merge service received FinalizeShuffleMerge RPC from driver, it will mark the MergePartitionsInfo as finilized instead of remove it.
Delete the shuffle merge files if partition.mapTracker is empty.
When spark driver begin to cleanup shuffle, if push-based shuffle is enabled, spark driver will send RemoveShuffleMerge RPC to all merger locations to delete all merge data files.

Why are the changes needed?
There will be too many merged shuffle data files for long running spark applications.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Local cluster test.

+CC @akpatnam25, @otterc - since you were planning to work on this already.

Hi, @mridulm Thanks for your review.
In my opinion, I do not think we should add ShuffleMergeId in RemoveShuffleMerge.
The push-based shuffle service will only keep the latest shuffleMergeId data for each shuffle, and RemoveShuffleMerge will cleanup its data.
Just like cleaning up normal shuffle data written by mapTasks,  these methods only need shuffleId: def removeShuffle(shuffleId: Int)
What do you think?

We should decouple current implementation details when making protocol changes, and make it extensible for future evolution.
In this case though, it is much more straightforward - there is an existing usecase which requires shuffle merge id.
When retrying an indeterminate stage, we should cleanup merged shuffle data for previous stage attempt (in submitMissingTasks, before unregisterAllMapAndMergeOutput) - and given the potential race conditions there, we dont want RemoveShuffleMerge to clean up for the next attempt (when we add support for this).
This specific change can be done in a follow up PR though - I want to get the basic mechanics working in this PR, and ensure the cleanup usecase is handled - before looking at further enhancements.

We should decouple current implementation details when making protocol changes, and make it extensible for future evolution.
In this case though, it is much more straightforward - there is an existing usecase which requires shuffle merge id. When retrying an indeterminate stage, we should cleanup merged shuffle data for previous stage attempt (in submitMissingTasks, before unregisterAllMapAndMergeOutput) - and given the potential race conditions there, we dont want RemoveShuffleMerge to clean up for the next attempt (when we add support for this).
This specific change can be done in a follow up PR though - I want to get the basic mechanics working in this PR, and ensure the cleanup usecase is handled - before looking at further enhancements.

Since the push-based shuffle service will auto clean up the old shuffle merge data,  so we don't need send RemoveShuffleMerge RPC for a new ShuffleMerge?
The only scenario I can think of now where a cleanup RPC needs is the spark job completes. Could we think of other scenarios?

retest this please

Can one of the admins verify this patch?

Thanks @mridulm , I will the the shuffleMergeId into the protocol later.
This PR also has some other issues, I will fix it soon.

Gentle ping @wankunde. Do you think you can update the PR soon? Please let us know if you need any help.

Gentle ping @wankunde. Do you think you can update the PR soon? Please let us know if you need any help.

Sorry for the late reply, I will update this PR today.

Hi, @otterc @mridulm If the spark application was stopped unexpected, there will be some leak merge files. In our production cluster, yarn nodeManager will cleanup those leak files if the disk is not full, and will not cleanup them if the disk is full.
Could we change the blockHandler.applicationRemoved(appId, false /* clean up local dirs */) to blockHandler.applicationRemoved(appId, true) in  YarnShuffleService to clean up them ?
https://github.com/apache/spark/blob/e6699570becadb91695572bca5adc1605dc1b2a8/common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java#L433

I will try to get to this later this week, do let me know if you are still working on it/have pending comments to address. Thanks

Hi, @mridulm , I've been working on some other issues recently. If @yabola  can do all or part of this task in https://github.com/apache/spark/pull/38560, please go ahead.

Hi, @mridulm @yabola I have updated this PR, could you help to this PR again? Thanks.

https://github.com/apache/spark/pull/37922#discussion_r1054110064
For the AppShufflePartitionInfo that is processing the first pushed shuffle block, partition.mapTracker.getCardinality() == 0, and won't put this reduceId to reduceIds, and then the shuffle files will not be cleanup and leak.
I prefer to add this change in this PR.

Could you add some unit test for this? I wrote unit test before. You can have a look or write new UT.

Could you add some unit test for this? I wrote unit test before. You can have a look or write new UT.

Thanks, backport the UT

+CC @otterc, can you take a look at this PR ?

@wankunde, please ping me when you are done with the updates to the PR. Thanks

@mridulm I rebase the code, could you help to review this PR again? Thanks

I just merged #39316 to unblock this PR, can you update to latest please ? Thx

@otterc Thanks for your review. Format the code.

The tests are failing @wankunde, though I dont think it is due to your PR.
Can you please take a look ? And retrigger it if it is unrelated ? Thanks !

Merged to master.
Thanks for fixing this @wankunde !
Thanks for the reviews @otterc, @akpatnam25, @yabola :-)

authors: AmplabJenkins mridulm otterc wankunde yabola


text:
[SPARK-32288][UI] Add failure summary for failed tasks in stage page

What changes were proposed in this pull request?
Add failure summary table for failed tasks in stage page.
Before:

After:

Why are the changes needed?
When there're many task failure during one stage, it's hard to find failure pattern such as aggregation task failure by exception type and message. If we have such information, we can easily know which type of exception of failure is the root cause of stage failure.
Does this PR introduce any user-facing change?
Yes
How was this patch tested?
Added UT in UISeleniumSuite

cc @gengliangwang and @sarutak FYI

Can one of the admins verify this patch?

+CC @shardulm94 , @thejdeep

@gengliangwang and @sarutak FYI @dongjoon-hyun Could you help take a look?

authors: AmplabJenkins HyukjinKwon mridulm warrenzhu25


text:
[SPARK-44053][BUILD] Update ORC to 1.8.4

What changes were proposed in this pull request?

This PR aims to update ORC to 1.8.4.
Why are the changes needed?

This will bring the following bug fixes.

https://issues.apache.org/jira/projects/ORC/versions/12353041

Does this PR introduce any user-facing change?

No.
How was this patch tested?

Pass the CIs.

cc @dongjoon-hyun

@guiyanakuang Please make the PR against the master branch.

@guiyanakuang Please make the PR against the master branch. You can change it here:

The plan after ORC community discussion is
Apache ORC 1.8.4 (2023-06-16, Yiqun Zhang) => Apache Spark 3.4.1
Apache ORC 1.9.0 (2023-06-30, Dongjoon Hyun) => Apache Spark 3.5.0
So I should have the right branch

Hi, @guiyanakuang and @wangyum .
Both of you are right and I'll release Apache ORC 1.9.0 in this month.
This is more like a timing issue where Apache Spark master branch is still using 1.8.3 because Apache ORC 1.9.0 is not released yet.
If Apache Spark master branch is already 1.9.0, this PR is correct.
So, @guiyanakuang , could you make another PR for master branch too? I can merge both PRs sequently from master first and branch-3.4 second.

I updated the title of this PR to [SPARK-44053][BUILD][3.4] Update ORC to 1.8.4.
For your new PR for master branch, please use [SPARK-44053][BUILD] Update ORC to 1.8.4.

I get it. Thanks @dongjoon-hyun and @wangyum

Merged to branch-3.4 for Apache Spark 3.4.0.
I merged https://github.com/apache/spark/pull/41593 first to master and merged this one.

Welcome to the Apache Spark community again, @guiyanakuang !

authors: dongjoon-hyun guiyanakuang wangyum


text:
[SPARK-4259][MLlib]: Add Power Iteration Clustering Algorithm with Gaussian Similarity Function

Add single pseudo-eigenvector PIC
Including documentations and updated pom.xml with the following codes:
mllib/src/main/scala/org/apache/spark/mllib/clustering/PIClustering.scala
mllib/src/test/scala/org/apache/spark/mllib/clustering/PIClusteringSuite.scala

Test build #26252 has started for   PR 4254 at commit 121e4d5.

This patch merges cleanly.

Test build #26252 has finished for   PR 4254 at commit 121e4d5.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26252/
Test FAILed.

Is it possible to do Gaussian similarity in another PR? It should be part of the feature transformation but not within PIC.

RE:

Is it possible to do Gaussian similarity in another PR? It should be part of the feature transformation but not within PIC. It would be easier for code review if the PR is minimal.

This is a big change late in the game.  We could do the following more easily:  change the signature to PIC to allow the input to be the Affinity Matrix (user defined).  Then we retain the code for the Gaussian similarity calculation to be sent in . Our tests  and documentation and verification all depend on this.

Hi,
We have suggestion here: to separate the creation/definition of the input graph from the PIC:
val G = PIC.createGaussianAffinityMatrix(sc, vertices)  // Create the affinity matrix in a Graph structure
val (ccenters, estCollected) = PIC.run(sc, G, nClusters, nIterations) // Run PIC

The new signatures are:
Take a Graph input instead of the input vertices:
def run(sc: SparkContext,
G: Graph[Double, Double],
nClusters: Int,
nIterations: Int = defaultIterations,
sigma: Double = defaultSigma,
minAffinity: Double = defaultMinAffinity,
nRuns: Int = defaultKMeansRuns)
And here is the new method for creating the input Graph:
def createGaussianAffinityMatrix(sc: SparkContext,
points: Points,
sigma: Double = defaultSigma,
minAffinity: Double = defaultMinAffinity)

Test build #26261 has started for   PR 4254 at commit 7ebd149.

This patch merges cleanly.

For spectral clustering, the input should be pairwise distances, e.g., RDD[(Long, Long, Double)]. It could be converted into a Graph easily with fromEdgeRDD. Besides, we can accept adjacency list RDD[(Long, Vector)] as input. How to compute the distance is definitely not part of spectral clustering and we should separate the implementation.

Test build #26261 has finished for   PR 4254 at commit 7ebd149.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26261/
Test PASSed.

I have moved out the Gaussian / Affinity matrix calculations.  It is not clear where their new home / if they have a new home. Presently the testcases rely upon them - so they are now in the testing suite.  The core PowerIterationClustering class is still working, now with a Graph input.

Test build #26272 has started for   PR 4254 at commit 92d4752.

This patch merges cleanly.

Test build #26272 has finished for   PR 4254 at commit 92d4752.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
  case class Params(

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26272/
Test FAILed.

Test build #26282 has started for   PR 4254 at commit c12dfc8.

This patch merges cleanly.

Test build #26282 has finished for   PR 4254 at commit c12dfc8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26282/
Test PASSed.

@fjiang6 @javadba Please focus on the public APIs first and then the implementation. The best way to check public APIs is generating the html doc and look what are exposed to users. We can probably refactor the test code in later PRs.

Test build #26354 has started for   PR 4254 at commit 24fbf52.

This patch merges cleanly.

Test build #26354 has finished for   PR 4254 at commit 24fbf52.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class PowerIterationClustering(

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26354/
Test PASSed.

Test build #26420 has started for   PR 4254 at commit f292f31.

This patch merges cleanly.

Test build #26423 has started for   PR 4254 at commit 4550850.

This patch merges cleanly.

Test build #26420 has finished for   PR 4254 at commit f292f31.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class PowerIterationClusteringModel(

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26420/
Test PASSed.

Test build #26423 has finished for   PR 4254 at commit 4550850.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class PowerIterationClusteringModel(

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26423/
Test PASSed.

LGTM except minor user guide issues, which will be addressed in SPARK-5503. I've merged this into master. Thanks for the contributing! (Now MLlib depends on GraphX.)

authors: AmplabJenkins SparkQA fjiang6 javadba mengxr


text:
[BUILD] Add the ability to launch spark-shell from SBT.

Now you can quickly launch the spark-shell without building an assembly.  For quick development iteration run build/sbt ~sparkShell and calling exit will relaunch with any changes.

Test build #26949 has started for   PR 4438 at commit b4e44fe.

This patch merges cleanly.

This is great to have.

Test build #26949 has finished for   PR 4438 at commit b4e44fe.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26949/
Test FAILed.

test this please

Test build #26970 has started for   PR 4438 at commit b4e44fe.

This patch merges cleanly.

Test build #26970 has finished for   PR 4438 at commit b4e44fe.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/26970/
Test FAILed.

Test build #591 has started for   PR 4438 at commit b4e44fe.

This patch merges cleanly.

Test build #591 has finished for   PR 4438 at commit b4e44fe.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Thanks. Merging in master & branch-1.3.

authors: AmplabJenkins SparkQA marmbrus rxin


text:
[SPARK-46750][CONNECT][PYTHON] DataFrame APIs code clean up

What changes were proposed in this pull request?
1, unify the import;
2, delete unused helper functions and variables;
Why are the changes needed?
code clean up
Does this PR introduce any user-facing change?
no
How was this patch tested?
ci
Was this patch authored or co-authored using generative AI tooling?
no

added the jira id, thanks @dongjoon-hyun and @HyukjinKwon
merged to master


text:
[SPARK-46894][PYTHON] Move PySpark error conditions into standalone JSON file

What changes were proposed in this pull request?
Move PySpark error conditions into a standalone JSON file.
Adjust the instructions in MANIFEST.in so they package the new JSON file correctly.
Why are the changes needed?
Having the JSON in its own file enables better IDE support for editing and managing the JSON. For example, VS Code will detect duplicate keys automatically and underline them.
This change also simplifies the logic to regenerate the JSON.
Does this PR introduce any user-facing change?
No.
How was this patch tested?

Existing unit tests.

I confirmed that rewriting the JSON file works as intended:
from pyspark.errors.exceptions import _write_self
_write_self()

I built a PySpark distribution, installed it, and confirmed the error class test still works.
trash dist/
./make-distribution.sh --pip -DskipTests -Phive
find dist -name 'error-conditions.json'
# dist/python/pyspark/errors/error-conditions.json
cd dist/python
python -m venv venv
source venv/bin/activate
pip install .
python pyspark/errors/tests/test_errors.py

Was this patch authored or co-authored using generative AI tooling?
No.

@itholic can you take a look please? I remember you took a look and failed to find a good way to upload JSON.

IIRC there was no major issue with managing the json itself. However, since we cannot integrate with the error-classes.json file on the JVM side - because we didn't want to have a JVM dependency -, we simply adopted a .py file that is more convenient way to manage in Python.
So I agree to change to a json file if the advantage of using a json file over using a .py file is clear, and if there are no issues with packaging. Also you might need to take a deeper look at the documentation. For example we're using .py file to build documentation for Error classes in PySpark.

since we cannot integrate with the error-classes.json file on the JVM side

This is for a separate potential PR, but if it were possible to use the "main" error JSON files from Scala in PySpark automatically, would we want to do that? I see that PySpark's errors don't define a SQLSTATE, so I assumed they were a separate thing and we didn't want to reuse the main error definitions.

So I agree to change to a json file if the advantage of using a json file over using a py file is clear, and if there are no issues with packaging. Also you might need to take a deeper look at the documentation. For example we're pointing the py file path from Error classes in PySpark.

Is this a reference to this command? https://github.com/apache/spark/blob/8060e7e73170c0122acb2a005f3c54487e226208/python/docs/source/conf.py#L42
Anyway, just FYI I am in the middle of revamping the main error documentation to make it easier to maintain. First I am cleaning up the terminology in #44902 but then I will consolidating the various sql-error-* pages. I will tag you in those PRs when I open them.

The build is still running, but the pyspark-core tests are passing. I believe importlib.resources is what we need to load data files packaged in the distribution we upload to PyPI.

This is for a separate potential PR, but if it were possible to use the "main" error JSON files from Scala in PySpark automatically, would we want to do that?

I don't think so. As I recall, the main reason for not doing it was because, as you said, the error structure on the PySpark side is different from the error structure on the JVM side.

I will be consolidating the various sql-error-* pages. I will tag you in those PRs when I open them.

+1

Is this a reference to this command?

Yes, so you might need to fix the description from https://github.com/apache/spark/blob/master/python/pyspark/errors_doc_gen.py#L44.

I think we should wait for the conversation in SPARK-46810 to resolve before merging this in.
But apart from that, is there anything more you'd like me to check here? Do you approve of the use of importlib.resources (which I think is the "correct" solution in our case)?

Converting to draft until SPARK-46810 is resolved.

We have agreed in SPARK-46810 to rename "error class" to "error condition", so this PR is unblocked since we know we won't need to rename the new error-classes.json file.
The work to rename all instances of "error class" to "error condition" across the board will happen in SPARK-46810 and SPARK-47429. I would like to keep this PR focused on simply moving the Python error conditions into a JSON file.
@HyukjinKwon - I believe this PR is ready to go. Do you have any oustanding concerns?

Just to make sure, does it work when you install PySpark as a ZIP file? e.g., downloading it from https://spark.apache.org/downloads.html would install PySpark as a ZIP file.

It's the one python/lib/pyspark.zip when you finish building.

Hmm, so when people install this ZIP how exactly do they do it? Because it does not install cleanly like the ZIP under python/dist/.
$ pip install .../spark/python/lib/pyspark.zip
Processing .../spark/python/lib/pyspark.zip
ERROR: file:///.../spark/python/lib/pyspark.zip does not appear to be a Python project:
  neither 'setup.py' nor 'pyproject.toml' found.

People can actually directly use PySpark via importing pyspark.zip, see https://spark.apache.org/docs/latest/api/python/getting_started/install.html?highlight=pythonpath#manually-downloading

@HyukjinKwon - Anything else you'd like to see done here?

Friendly ping @HyukjinKwon.

Merged to master.

authors: HyukjinKwon itholic nchammas


text:
[SPARK-6473] [core] Do not try to figure out Scala version if not needed...

....

Test build #29027 has started for   PR 5143 at commit a2e5e2d.

This patch merges cleanly.

Seems fine to me.

Test build #29027 has finished for   PR 5143 at commit a2e5e2d.

This patch passes all tests.

This patch merges cleanly.

This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29027/
Test PASSed.

authors: AmplabJenkins SparkQA srowen vanzin


text:
[SPARK-5931][CORE] Use consistent naming for time properties

I've added new utility methods to do the conversion from times specified as e.g. 120s, 240ms, 360us to convert to a consistent internal representation. I've updated usage of these constants throughout the code to be consistent.
I believe I've captured all usages of time-based properties throughout the code. I've also updated variable names in a number of places to reflect their units for clarity and updated documentation where appropriate.

Test build #29325 has started for   PR 5236 at commit 59bf9e1.

This patch merges cleanly.

Test build #29326 has started for   PR 5236 at commit 4933fda.

This patch merges cleanly.

Test build #29328 has started for   PR 5236 at commit 064ebd6.

This patch merges cleanly.

Test build #29329 has started for   PR 5236 at commit cde9bff.

This patch merges cleanly.

Test build #29330 has started for   PR 5236 at commit 42477aa.

This patch merges cleanly.

Test build #29325 has finished for   PR 5236 at commit 59bf9e1.

This patch fails Spark unit tests.

This patch merges cleanly.

This patch adds the following public classes (experimental):

class Normalizer extends UnaryTransformer[Vector, Vector, Normalizer]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29325/
Test FAILed.

Test build #29329 has finished for   PR 5236 at commit cde9bff.

This patch fails Spark unit tests.

This patch merges cleanly.

This patch adds the following public classes (experimental):

class Normalizer extends UnaryTransformer[Vector, Vector, Normalizer]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29329/
Test FAILed.

Test build #29330 has finished for   PR 5236 at commit 42477aa.

This patch fails Spark unit tests.

This patch merges cleanly.

This patch adds the following public classes (experimental):

class Normalizer extends UnaryTransformer[Vector, Vector, Normalizer]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29330/
Test FAILed.

Test build #29326 has finished for   PR 5236 at commit 4933fda.

This patch passes all tests.

This patch merges cleanly.

This patch adds the following public classes (experimental):

class Normalizer extends UnaryTransformer[Vector, Vector, Normalizer]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29326/
Test PASSed.

Test build #29328 has finished for   PR 5236 at commit 064ebd6.

This patch fails Spark unit tests.

This patch merges cleanly.

This patch adds the following public classes (experimental):

class Normalizer extends UnaryTransformer[Vector, Vector, Normalizer]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29328/
Test FAILed.

retest this please

Test build #29331 has started for   PR 5236 at commit 9a29d8d.

This patch merges cleanly.

Test build #29331 has finished for   PR 5236 at commit 9a29d8d.

This patch fails Spark unit tests.

This patch merges cleanly.

This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29331/
Test FAILed.

retest this please

retest this please

Test build #29339 has started for   PR 5236 at commit 8f741e1.

This patch merges cleanly.

Test build #29339 has finished for   PR 5236 at commit 8f741e1.

This patch fails Spark unit tests.

This patch merges cleanly.

This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29339/
Test FAILed.

This change actually stops supporting a bunch of existing config properties, right? I don't think we can do that.
As discussed in https://issues.apache.org/jira/browse/SPARK-3859 it's possible to deprecate the existing ones, add new ones, and support both. In practice, the deprecated configs will stick around a long time. (cf. MapReduce and it's "deprecated" properties from like 20 years ago).
I personally am not sure about changing the config properties just for this as a result. However I can see changing some internal variable names to reflect their units.
CC @andrewor14

Yeah, the issue is changing the property names, not adding to the values they can take on. I think that's a decent idea myself, to be able to specify "100s" instead of "100000" for a millisecond property.
It raises interesting questions. Is it weird to see foo.bar.timeoutMS = 100s ? Of course, that's the reason for changing the property name, but then that itself is disruptive. Also, now you could specify foo.bar.intervalSec = 1ms whereas it would not have been possible before to specify an interval below 1 second. I suspect that will be OK, with code changes, but bears verifying.
I swear there was another PR for just this, but I can't find it for the life of me.
This is one where I'd appeal to others for an opinion.

Sean - there's actually only one variable that is still labeled as Ms. All
the other ones don't include units in the parameter name.
On Sun, Mar 29, 2015 at 4:11 AM Sean Owen notifications@github.com wrote:

Yeah, the issue is changing the property names, not adding to the values
they can take on. I think that's a decent idea myself, to be able to
specify "100s" instead of "100000" for a millisecond property.
It raises interesting questions. Is it weird to see foo.bar.timeoutMS =
100s ? Of course, that's the reason for changing the property name, but
then that itself is disruptive. Also, now you could specify foo.bar.intervalSec
= 1ms whereas it would not have been possible before to specify an
interval below 1 second. I suspect that will be OK, with code changes, but
bears verifying.
I swear there was another PR for just this, but I can't find it for the
life of me.
This is one where I'd appeal to others for an opinion.
—
Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/5236#issuecomment-87395279.

If the two in the JIRA are all there are, then yea that's not much to worry about, probably. I still think that it's not worth adding a suffix to existing properties though.

Test build #29409 has started for   PR 5236 at commit 499bdf0.

Test build #29411 has started for   PR 5236 at commit 3a12dd8.

Test build #29413 has started for   PR 5236 at commit 68f4e93.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29409/
Test FAILed.

Test build #29411 has finished for   PR 5236 at commit 3a12dd8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29411/
Test FAILed.

retest this please

Test build #29417 has started for   PR 5236 at commit 70ac213.

Test build #29413 has finished for   PR 5236 at commit 68f4e93.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29413/
Test FAILed.

Test build #29417 has finished for   PR 5236 at commit 70ac213.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29417/
Test PASSed.

@vanzin I've updated the Scala to use a map like you suggested, still need to update the Java side to do the same if this works. Does this look reasonable? Thanks!

Test build #29466 has started for   PR 5236 at commit 647b5ac.

Test build #29466 has finished for   PR 5236 at commit 647b5ac.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29466/
Test FAILed.

Test build #29467 has started for   PR 5236 at commit 1c0c07c.

Test build #29467 timed out     for PR 5236 at commit 1c0c07c     after a configured wait of 120m.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29467/
Test FAILed.

Test build #29485 has started for   PR 5236 at commit bac9edf.

retest this please

Test build #29490 has started for   PR 5236 at commit 1858197.

Test build #29490 has finished for   PR 5236 at commit 1858197.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29490/
Test FAILed.

Test build #29485 timed out     for PR 5236 at commit bac9edf     after a configured wait of 120m.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29485/
Test FAILed.

retest this please

Test build #29494 has started for   PR 5236 at commit 3b126e1.

Looks OK to me, left some minor comments and suggestions.
Could you also add unit tests to UtilsSuite.scala?

Test build #29494 has finished for   PR 5236 at commit 3b126e1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29494/
Test PASSed.

Latests fixes merged. Unit tests added. Thanks!

retest this please

Test build #29502 has started for   PR 5236 at commit 39164f9.

Test build #29502 has finished for   PR 5236 at commit 39164f9.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29502/
Test FAILed.

Test build #29503 has started for   PR 5236 at commit 39164f9.

Test build #29503 has finished for   PR 5236 at commit 39164f9.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29503/
Test FAILed.

Test build #29504 has started for   PR 5236 at commit b2fc965.

Some more nits, but looks good. It would be nice to have a JavaUtilsSuite - it sucks that this code has to exist in two different places, but it's the price to pay I guess. :-/
I wonder if people would be ok with Utils.scala calling into JavaUtils.

Test build #29504 has finished for   PR 5236 at commit b2fc965.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29504/
Test PASSed.

I wouldn't mind calling into the Java for this. I think it pays to avoid code duplication.

I added the call into Java - this way we also don't need a separate test since it's all calling the same code.

Test build #29508 has started for   PR 5236 at commit dd0a680.

Test build #29508 has finished for   PR 5236 at commit dd0a680.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29508/
Test FAILed.

Test build #29510 has started for   PR 5236 at commit bf779b0.

Test build #29510 has finished for   PR 5236 at commit bf779b0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29510/
Test FAILed.

retest this please

Test build #29516 has started for   PR 5236 at commit bf779b0.

Test build #29516 has finished for   PR 5236 at commit bf779b0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29516/
Test PASSed.

Last batch of comments incorporated. Thanks for the thorough review, Marcelo!

Test build #29524 has started for   PR 5236 at commit 76cfa27.

Test build #29524 has finished for   PR 5236 at commit 76cfa27.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29524/
Test PASSed.

Thanks for working on this @ilganeli. I will take a look at this in the next day or two.

@andrewor14 Thanks man - let me know if it needs anything else. I didn't end up changing any of the names for existing configs or introducing new versions of those but I think that's ok for time parameters because there's only a couple with explicit suffixes. I think it could be done but it's a bit of a pain in the neck and would complicate the code for ingesting the modified parameters (since we'd now need to check for both A and B). SPARK-5932 would be worse since there's many more fields with explicit suffixes there.

Test build #29739 has started for   PR 5236 at commit 5193d5f.

Test build #29739 has finished for   PR 5236 at commit 5193d5f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29739/
Test PASSed.

Test build #29743 has started for   PR 5236 at commit 6387772.

Test build #29743 has finished for   PR 5236 at commit 6387772.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29743/
Test FAILed.

Test build #29747 has started for   PR 5236 at commit ff40bfe.

Test build #29747 has finished for   PR 5236 at commit ff40bfe.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29747/
Test PASSed.

Test build #29748 has started for   PR 5236 at commit 1465390.

Test build #29748 has finished for   PR 5236 at commit 1465390.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29748/
Test PASSed.

Test build #29797 has started for   PR 5236 at commit cbf41db.

Test build #29797 has finished for   PR 5236 at commit cbf41db.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29797/
Test PASSed.

@ilganeli I took a quick look at this and I believe this does not currently handle backward compatibility. For instance, spark.storage.blockManagerSlaveTimeoutMs used to take in the string "5000", but now I have to do "5s" otherwise it will throw an exception. The bigger issue is that it's very strange to see a config ending with Ms to accept a value "5s" or "20ms". I think this patch does only half of what I intended, and the other half must accompany this half.
However, to deprecate all these configs is currently a very verbose process. This is why I opened SPARK-5933, which I actually see as a prerequisite before we solve either SPARK-5931 or SPARK-5932. For these reasons I would suggest that we put this PR on hold for now. I definitely do want to merge some variant of the changes here, but I don't think there's currently a great way forward yet.

For instance, spark.storage.blockManagerSlaveTimeoutMs used to take in the string 5000, but now I have to pass it 5s otherwise it will throw an exception.

I don't believe that's true; unless the code changed since I last read it, setting it to 5000 should yield the correct result too.

Andrew - if you pass the string 5000 it will be interpreted as 5000ms as intended. I use 5s as shorthand in the code for brevity. I also believe blockTimeoutMs is the only one that's explicitly named with units. Other ones are not, so deprecating just that field shouldn't be difficult.
Sent with Good (www.good.com)
-----Original Message-----
From: andrewor14 [notifications@github.commailto:notifications@github.com]
Sent: Tuesday, April 07, 2015 03:56 PM Eastern Standard Time
To: apache/spark
Cc: Ganelin, Ilya
Subject: Re: [spark] [SPARK-5931][CORE] Use consistent naming for time properties (#5236)
@ilganelihttps://github.com/ilganeli I took a quick look at this and I believe this does not currently handle backward compatibility. For instance, spark.storage.blockManagerSlaveTimeoutMs used to take in the string "5000", but now I have to do "5s" otherwise it will throw an exception. The bigger issue is that it's very strange to see a config ending with Ms to accept a value "5s" or "20ms". I think this patch does only half of what I intended, and the other half must accompany this half.
However, to deprecate all these configs is currently a very verbose process. This is why I opened SPARK-5933https://issues.apache.org/jira/browse/SPARK-5933, which I actually see as a prerequisite before we solve either SPARK-5931 or SPARK-5932. For these reasons I would suggest that we put this PR on hold for now. I definitely do want to merge some variant of the changes here, but I don't think there's currently a great way forward yet.
—
Reply to this email directly or view it on GitHubhttps://github.com/apache/spark/pull/5236#issuecomment-90713134.

The information contained in this e-mail is confidential and/or proprietary to Capital One and/or its affiliates. The information transmitted herewith is intended only for use by the individual or entity to which it is addressed.  If the reader of this message is not the intended recipient, you are hereby notified that any review, retransmission, dissemination, distribution, copying or other use of, or taking of any action in reliance upon this information is strictly prohibited. If you have received this communication in error, please contact the sender and delete the material from your computer.

@vanzin @ilganeli Oh I see, I didn't realize that 5000 would still work. Then it seems fine. I plan to look at this in much closer detail today or tomorrow to make sure that we haven't missed any configs, but from first glance it seems that we need to at least fix "spark.yarn.scheduler.heartbeat.interval-ms" (from the JIRA). Maybe there are a few others.

Test build #29875 has started for   PR 5236 at commit d4efd26.

Test build #29879 has started for   PR 5236 at commit 1a1122c.

Test build #29879 has finished for   PR 5236 at commit 1a1122c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29879/
Test FAILed.

Test build #29880 has started for   PR 5236 at commit cbd2ca6.

Test build #29875 has finished for   PR 5236 at commit d4efd26.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29875/
Test PASSed.

@ilganeli this change looks great! Aside from the smaller comments I left inline, I have two high level suggestions. The first is documentation. Without this there is no way for the user to find out that they can use this new notation for these configs. We should update the default values for all those configs in configuration.md to use this notation, have a new section that fully delineates all the options available.
The second is that I notice we do Utils.getTimeAsS(conf.get(...)) a lot. Since these conversions are designed for configs, would it make sense to have conf.getSeconds and conf.getMicroseconds as well?

@andrewor14 W.r.t to documentation I had originally added a note in the configuration document pointing out how time properties should be treated differently. I think that got lost in the shuffle, so I'll restore that, and I can update the default values for time properties to include the string. I like the idea of adding methods to the SparkConfig class - I'll do that.

Test build #29880 has finished for   PR 5236 at commit cbd2ca6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29880/
Test PASSed.

Test build #29889 has started for   PR 5236 at commit 6f651a8.

@vanzin - Fixed the potential NPE. Sorry, didn't know about that behavior of containsKey.

Test build #29892 has started for   PR 5236 at commit 7d19cdd.

Test build #29889 has finished for   PR 5236 at commit 6f651a8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29889/
Test FAILed.

@ilganeli you should add a unit test for that. It seems there are no unit tests for suffix-less strings.

There is. The first test for every case has no suffix.
Sent with Good (www.good.com)
-----Original Message-----
From: Marcelo Vanzin [notifications@github.commailto:notifications@github.com]
Sent: Wednesday, April 08, 2015 05:53 PM Eastern Standard Time
To: apache/spark
Cc: Ganelin, Ilya
Subject: Re: [spark] [SPARK-5931][CORE] Use consistent naming for time properties (#5236)
@ilganelihttps://github.com/ilganeli you should add a unit test for that. It seems there are no unit tests for suffix-less strings.
—
Reply to this email directly or view it on GitHubhttps://github.com/apache/spark/pull/5236#issuecomment-91046918.

The information contained in this e-mail is confidential and/or proprietary to Capital One and/or its affiliates. The information transmitted herewith is intended only for use by the individual or entity to which it is addressed.  If the reader of this message is not the intended recipient, you are hereby notified that any review, retransmission, dissemination, distribution, copying or other use of, or taking of any action in reliance upon this information is strictly prohibited. If you have received this communication in error, please contact the sender and delete the material from your computer.

There is. The first test for every case has no suffix.

Ah, my bad. In that case the previous code was fine. (just tried and HashMap.containsKey(null) works.)

Test build #29892 has finished for   PR 5236 at commit 7d19cdd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29892/
Test FAILed.

Test build #29899 has started for   PR 5236 at commit 69fedcc.

Test build #29899 has finished for   PR 5236 at commit 69fedcc.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29899/
Test FAILed.

Test build #29913 has started for   PR 5236 at commit 8927e66.

Test build #29913 has finished for   PR 5236 at commit 8927e66.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29913/
Test PASSed.

Test build #29924 has started for   PR 5236 at commit 642a06d.

Test build #29924 has finished for   PR 5236 at commit 642a06d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29924/
Test PASSed.

@andrewor14 Is this in a good state now?

Test build #30079 has started for   PR 5236 at commit 25d3f52.

Test build #30079 has finished for   PR 5236 at commit 25d3f52.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30079/
Test PASSed.

All changes pulled in - I think all parties should be happy now :)

Test build #30138 has started for   PR 5236 at commit bc04e05.

Test build #30138 has finished for   PR 5236 at commit bc04e05.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30138/
Test PASSed.

LGTM. Thanks for your persistence. Let me leave this for another day for further comments.

Test build #30176 has started for   PR 5236 at commit 951ca2d.

Test build #30176 has finished for   PR 5236 at commit 951ca2d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30176/
Test FAILed.

Test build #30179 has started for   PR 5236 at commit f5fafcd.

Test build #30179 has finished for   PR 5236 at commit f5fafcd.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30179/
Test FAILed.

I just took a quick look at the existing configs that have units. It appears that none of them are documented, fortunately. I think this patch is more or less ready for merge once it passes tests.

Test build #30184 has started for   PR 5236 at commit de3bff9.

Test build #30184 has finished for   PR 5236 at commit de3bff9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch removes the following dependencies:

RoaringBitmap-0.4.5.jar
activation-1.1.jar
akka-actor_2.10-2.3.4-spark.jar
akka-remote_2.10-2.3.4-spark.jar
akka-slf4j_2.10-2.3.4-spark.jar
aopalliance-1.0.jar
arpack_combined_all-0.1.jar
avro-1.7.7.jar
breeze-macros_2.10-0.11.2.jar
breeze_2.10-0.11.2.jar
chill-java-0.5.0.jar
chill_2.10-0.5.0.jar
commons-beanutils-1.7.0.jar
commons-beanutils-core-1.8.0.jar
commons-cli-1.2.jar
commons-codec-1.10.jar
commons-collections-3.2.1.jar
commons-compress-1.4.1.jar
commons-configuration-1.6.jar
commons-digester-1.8.jar
commons-httpclient-3.1.jar
commons-io-2.1.jar
commons-lang-2.5.jar
commons-lang3-3.3.2.jar
commons-math-2.1.jar
commons-math3-3.1.1.jar
commons-net-2.2.jar
compress-lzf-1.0.0.jar
config-1.2.1.jar
core-1.1.2.jar
curator-client-2.4.0.jar
curator-framework-2.4.0.jar
curator-recipes-2.4.0.jar
gmbal-api-only-3.0.0-b023.jar
grizzly-framework-2.1.2.jar
grizzly-http-2.1.2.jar
grizzly-http-server-2.1.2.jar
grizzly-http-servlet-2.1.2.jar
grizzly-rcm-2.1.2.jar
groovy-all-2.3.7.jar
guava-14.0.1.jar
guice-3.0.jar
hadoop-annotations-2.2.0.jar
hadoop-auth-2.2.0.jar
hadoop-client-2.2.0.jar
hadoop-common-2.2.0.jar
hadoop-hdfs-2.2.0.jar
hadoop-mapreduce-client-app-2.2.0.jar
hadoop-mapreduce-client-common-2.2.0.jar
hadoop-mapreduce-client-core-2.2.0.jar
hadoop-mapreduce-client-jobclient-2.2.0.jar
hadoop-mapreduce-client-shuffle-2.2.0.jar
hadoop-yarn-api-2.2.0.jar
hadoop-yarn-client-2.2.0.jar
hadoop-yarn-common-2.2.0.jar
hadoop-yarn-server-common-2.2.0.jar
ivy-2.4.0.jar
jackson-annotations-2.4.0.jar
jackson-core-2.4.4.jar
jackson-core-asl-1.8.8.jar
jackson-databind-2.4.4.jar
jackson-jaxrs-1.8.8.jar
jackson-mapper-asl-1.8.8.jar
jackson-module-scala_2.10-2.4.4.jar
jackson-xc-1.8.8.jar
jansi-1.4.jar
javax.inject-1.jar
javax.servlet-3.0.0.v201112011016.jar
javax.servlet-3.1.jar
javax.servlet-api-3.0.1.jar
jaxb-api-2.2.2.jar
jaxb-impl-2.2.3-1.jar
jcl-over-slf4j-1.7.10.jar
jersey-client-1.9.jar
jersey-core-1.9.jar
jersey-grizzly2-1.9.jar
jersey-guice-1.9.jar
jersey-json-1.9.jar
jersey-server-1.9.jar
jersey-test-framework-core-1.9.jar
jersey-test-framework-grizzly2-1.9.jar
jets3t-0.7.1.jar
jettison-1.1.jar
jetty-util-6.1.26.jar
jline-0.9.94.jar
jline-2.10.4.jar
jodd-core-3.6.3.jar
json4s-ast_2.10-3.2.10.jar
json4s-core_2.10-3.2.10.jar
json4s-jackson_2.10-3.2.10.jar
jsr305-1.3.9.jar
jtransforms-2.4.0.jar
jul-to-slf4j-1.7.10.jar
kryo-2.21.jar
log4j-1.2.17.jar
lz4-1.2.0.jar
management-api-3.0.0-b012.jar
mesos-0.21.0-shaded-protobuf.jar
metrics-core-3.1.0.jar
metrics-graphite-3.1.0.jar
metrics-json-3.1.0.jar
metrics-jvm-3.1.0.jar
minlog-1.2.jar
netty-3.8.0.Final.jar
netty-all-4.0.23.Final.jar
objenesis-1.2.jar
opencsv-2.3.jar
oro-2.0.8.jar
paranamer-2.6.jar
parquet-column-1.6.0rc3.jar
parquet-common-1.6.0rc3.jar
parquet-encoding-1.6.0rc3.jar
parquet-format-2.2.0-rc1.jar
parquet-generator-1.6.0rc3.jar
parquet-hadoop-1.6.0rc3.jar
parquet-jackson-1.6.0rc3.jar
protobuf-java-2.4.1.jar
protobuf-java-2.5.0-spark.jar
py4j-0.8.2.1.jar
pyrolite-2.0.1.jar
quasiquotes_2.10-2.0.1.jar
reflectasm-1.07-shaded.jar
scala-compiler-2.10.4.jar
scala-library-2.10.4.jar
scala-reflect-2.10.4.jar
scalap-2.10.4.jar
scalatest_2.10-2.2.1.jar
slf4j-api-1.7.10.jar
slf4j-log4j12-1.7.10.jar
snappy-java-1.1.1.6.jar
spark-bagel_2.10-1.4.0-SNAPSHOT.jar
spark-catalyst_2.10-1.4.0-SNAPSHOT.jar
spark-core_2.10-1.4.0-SNAPSHOT.jar
spark-graphx_2.10-1.4.0-SNAPSHOT.jar
spark-launcher_2.10-1.4.0-SNAPSHOT.jar
spark-mllib_2.10-1.4.0-SNAPSHOT.jar
spark-network-common_2.10-1.4.0-SNAPSHOT.jar
spark-network-shuffle_2.10-1.4.0-SNAPSHOT.jar
spark-repl_2.10-1.4.0-SNAPSHOT.jar
spark-sql_2.10-1.4.0-SNAPSHOT.jar
spark-streaming_2.10-1.4.0-SNAPSHOT.jar
spire-macros_2.10-0.7.4.jar
spire_2.10-0.7.4.jar
stax-api-1.0.1.jar
stream-2.7.0.jar
tachyon-0.5.0.jar
tachyon-client-0.5.0.jar
uncommons-maths-1.2.2a.jar
unused-1.0.0.jar
xmlenc-0.52.jar
xz-1.0.jar
zookeeper-3.4.5.jar

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30184/
Test PASSed.

Looking strong. I think that's all the comments addressed now. Going once, going twice?

Test build #30204 has started for   PR 5236 at commit 4526c81.

Ok, LGTM I'm merging this into master. It's great to see things on your todo list getting ticked off by others in the community. Thanks @ilganeli @srowen @vanzin!

Test build #30204 has finished for   PR 5236 at commit 4526c81.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30204/
Test PASSed.

I'm late to the party, but just want to say -- this is super cool!

Hah. Thanks.
Sent with Good (www.good.com)
-----Original Message-----
From: Reynold Xin [notifications@github.commailto:notifications@github.com]
Sent: Wednesday, April 29, 2015 01:36 AM Eastern Standard Time
To: apache/spark
Cc: Ganelin, Ilya
Subject: Re: [spark] [SPARK-5931][CORE] Use consistent naming for time properties (#5236)
I'm late to the party, but just want to say -- this is super cool!
—
Reply to this email directly or view it on GitHubhttps://github.com/apache/spark/pull/5236#issuecomment-97312212.

The information contained in this e-mail is confidential and/or proprietary to Capital One and/or its affiliates. The information transmitted herewith is intended only for use by the individual or entity to which it is addressed.  If the reader of this message is not the intended recipient, you are hereby notified that any review, retransmission, dissemination, distribution, copying or other use of, or taking of any action in reliance upon this information is strictly prohibited. If you have received this communication in error, please contact the sender and delete the material from your computer.

authors: AmplabJenkins SparkQA andrewor14 ilganeli rxin srowen vanzin


text:
The web page is not found about api docs

The url gaven about api is not found. For example:
the url of [Spark Scala API (Scaladoc)] is https://github.com/apache/spark/blob/master/docs/api/scala/index.html
, but this web page is not found. And the correct url is https://spark.apache.org/docs/latest/api/scala/index.html

Can one of the admins verify this patch?

@KaiXinXiaoLei  these docs are not for viewing on github. They are built using Jekyll and can be found online at: http://spark.apache.org/docs/latest/api.html

Yes, please close this PR

ok . i see.

authors: AmplabJenkins KaiXinXiaoLei rxin srowen