build: Separate and speedup TPC-DS benchmark #130
Conversation
advancedxy
changed the title
|
This might be reviewed and merged after #109 is merged. I tested on my local repo, it now take ~36min to finish TPC-DS benchmarks. |
| with: | ||
| rust-version: ${{env.RUST_VERSION}} | ||
| jdk-version: 11 | ||
| - name: Cache Maven dependencies |
There was a problem hiding this comment.
reuse Maven cache.
| repository: databricks/tpcds-kit | ||
| path: ./tpcds-kit | ||
| - name: Build Comet | ||
| run: make release |
There was a problem hiding this comment.
It's quite slow to build under release profile. We may find a way to speed up rust build.
But it's sufficient for now, we can do that later.
There was a problem hiding this comment.
I think it is mostly because we uses lto = "thin".
| - name: Run TPC-DS queries (Sort merge join) | ||
| if: matrix.join == 'sort_merge' | ||
| run: | | ||
| SPARK_HOME=`pwd` SPARK_TPCDS_DATA=`pwd`/tpcds-sf-1 ./mvnw -B -Prelease -Dsuites=org.apache.spark.sql.CometTPCDSQuerySuite test |
There was a problem hiding this comment.
-Prelease to include the release build lib.
And I noticed that the java tests are also executed but they are executed quickly.
|
Merged, thanks! |
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| name: Run TPC-DS Benchmark |
There was a problem hiding this comment.
This is not actually a benchmark, but correctness check (with Spark result).
There was a problem hiding this comment.
hmm, let me submit a follow up PR to fix it then..
There was a problem hiding this comment.
I thought about this too when reviewing this PR, but decided it is fine since we could re-purpose this workflow in future for both benchmarking and correctness check. It's fine for me if we decide to change it or not.
| /root/.m2/repository | ||
| key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-java-maven- |
There was a problem hiding this comment.
I wonder why we have a - postfix there?
There was a problem hiding this comment.
The key is ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }}, so the restored keys should be a prefix of that key.
See cache action's restore-keys(https://github.com/actions/cache) for explaination:
restore-keys - An ordered list of prefix-matched keys to use for restoring stale cache if no cache hit occurred for key.
There was a problem hiding this comment.
The - could be removed, but it would be natural to include that since it's in the prefix of the key.
There was a problem hiding this comment.
I wonder why we use this prefix restore key? If hashFiles is different, I think we don't want to restore it, right?
There was a problem hiding this comment.
If hashFiles is different, I think we don't want to restore it, right?
For maven dependencies, even if hashFiles is different, we may want to restore it as the previous cache should contains most of needed dependency. mvn will download missing jars automatically.
| path: | | ||
| ~/.m2/repository | ||
| /root/.m2/repository | ||
| key: ${{ runner.os }}-java-maven-${{ hashFiles('**/pom.xml') }} |
There was a problem hiding this comment.
Does Java version affect this maven cache?
We have os in key but seems Java version isn't there.
There was a problem hiding this comment.
I think it doesn't matter that much as most jars are java version agnostic?
And this cache will share the PR build's maven dependency..
Another thing to add: github's cache has quota limit: 10GB. The cached maven dependencies are already quite big(~1GB). If we add the java version to the key, it may create too much maven cache dependencies.
| - name: Restore TPC-DS generated data | ||
| id: cache-tpcds-sf-1 | ||
| uses: actions/cache@v4 |
There was a problem hiding this comment.
Can we just restore it using restore action?
There was a problem hiding this comment.
Can we just restore it using restore action?
Could you point me the link to the restore action? I searched github but didn't find one.
There was a problem hiding this comment.
There was a problem hiding this comment.
I see, thanks. Will fix it.
Which issue does this PR close?
Partially resolves #67
Rationale for this change
Reduce CI runtime and improve developer efficiency
What changes are included in this PR?
How are these changes tested?
Verified with CI runs