by default enable skip agg

[binmahone]

this PR closes #12557, pls check out the descriptions there

[binmahone]

build

[binmahone]

build

[abellina]

I believe we should mark the config internal as it doesn't seem like it is a user setting. You'd have to know that the partial aggregate has two stages, and that the first stage does some aggregation and that there are more stages that further combine these partial aggregates into bigger partial aggregates.

Side comment, this log at INFO level seems a little odd. Shouldn't this be at DEBUG level? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L995 and the INFO level one should be the case where we do skip? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L986

[binmahone]

I believe we should mark the config internal as it doesn't seem like it is a user setting. You'd have to know that the partial aggregate has two stages, and that the first stage does some aggregation and that there are more stages that further combine these partial aggregates into bigger partial aggregates.

will do

Side comment, this log at INFO level seems a little odd. Shouldn't this be at DEBUG level? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L995 and the INFO level one should be the case where we do skip? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L986

These two logs are both helpful when troubleshooting the skip agg case. How about I changing both to INFO level? I don't know what role DEBUG level logging are supposed to play for us. To me, it would be nice if a log in user production is informative enough for most troubleshootings. Asking user to run it again with DEBUG logging settings would be impractical sometimes because it might be very costly.

[binmahone]

I believe we should mark the config internal as it doesn't seem like it is a user setting. You'd have to know that the partial aggregate has two stages, and that the first stage does some aggregation and that there are more stages that further combine these partial aggregates into bigger partial aggregates.

will do

Side comment, this log at INFO level seems a little odd. Shouldn't this be at DEBUG level? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L995 and the INFO level one should be the case where we do skip? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L986

These two logs are both helpful when troubleshooting the skip agg case. How about I changing both to INFO level? I don't know what role DEBUG level logging are supposed to play for us. To me, it would be nice if a log in user production is informative enough for most troubleshootings (unless it's very verbose). Asking user to run it again with DEBUG logging settings would be impractical sometimes because it might be very costly.

[abellina]

I believe we should mark the config internal as it doesn't seem like it is a user setting. You'd have to know that the partial aggregate has two stages, and that the first stage does some aggregation and that there are more stages that further combine these partial aggregates into bigger partial aggregates.

will do

Side comment, this log at INFO level seems a little odd. Shouldn't this be at DEBUG level? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L995 and the INFO level one should be the case where we do skip? https://github.com/NVIDIA/spark-rapids/blob/branch-25.06/sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala#L986

These two logs are both helpful when troubleshooting the skip agg case. How about I changing both to INFO level? I don't know what role DEBUG level logging are supposed to play for us. To me, it would be nice if a log in user production is informative enough for most troubleshootings (unless it's very verbose). Asking user to run it again with DEBUG logging settings would be impractical sometimes because it might be very costly.

That's fine with me. It's once per task for the first batch only, so it should be fine for now to make them both INFO

[binmahone]

build

[binmahone]

build

[binmahone]

build