[SPARK-36821][SQL] Make class ColumnarBatch extendable - addendum

[flyrain]

What changes were proposed in this pull request?

A follow up of #34054. Three things changed:

1. Add a test for extendable class ColumnarBatch
2. Make ColumnarBatchRow public.
3. Change private fields to protected fields.

Why are the changes needed?

A follow up of #34054. Class ColumnarBatch need to be extendable to support better vectorized reading in multiple data sources. For example, Iceberg needs to filter out deleted rows in a batch before Spark consumes it, to support row-level delete( apache/iceberg#3141) in vectorized read.

Does this PR introduce any user-facing change?

No

How was this patch tested?

A new test is added

[dbtsai]

add to whitelist

[dbtsai]

okay to test

[dbtsai]

cc @sunchao @viirya @dongjoon-hyun @huaxingao

[viirya]

Shall we add @Evolving too?

[flyrain]

Made the change

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48107/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48107/

[SparkQA]

Test build #143595 has finished for PR 34087 at commit e626f9e.

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

[dongjoon-hyun]

cc @cloud-fan , @tgravescs

[flyrain]

cc @aokolnychyi

[sunchao]

This basically looks OK to me, although I'm thinking whether it's better to make ColumnarBatch an abstract class:

public abstract class AbstractColumnarBatch implements AutoCloseable, Iterable<InternalRow> {
  protected int numRows;
  public abstract ColumnVector[] columns();

  @Override
  public void close() {
    for (ColumnVector c: columns()) {
      c.close();
    }
  }

  public void setNumRows(int numRows) {
    this.numRows = numRows;
  }

  public int numRows() { return numRows; }
  public int numCols() { return columns().length; }
  public ColumnVector column(int ordinal) { return columns()[ordinal]; }
}

This way we'll give data source implementors more flexibilities for the internal stuff and we don't have to be tied with ColumnarBatchRow and expose it also.

[flyrain]

This basically looks OK to me, although I'm thinking whether it's better to make ColumnarBatch an abstract class:
This way we'll give data source implementors more flexibilities for the internal stuff and we don't have to be tied with ColumnarBatchRow and expose it also.

@sunchao, I'm OK with an abstract class. We may still expose ColumnarBatchRow since any class extending the abstract class still needs it. Of course, they can implement their own version, but the logic'd most likely be same. One question for your code snippet, we should add the public method rowIterator, right? It is the major interface of the ColumnarBatch.

[sunchao]

We may still expose ColumnarBatchRow since any class extending the abstract class still needs it

I'm thinking whether they can just use ColumnarRow instead.

One question for your code snippet, we should add the public method rowIterator, right? It is the major interface of the ColumnarBatch.

Yea. We can have the class implement Iterable<InternalRow> which is a more standard Java API. We'll need to replace all the places that use rowInterator with iterator though.

[dbtsai]

+1 on using Iterable<InternalRow>

[sunchao]

Actually it may not be so easy to use ColumnarRow, so I'm fine with exposing ColumnarBatchRow here. Eventually we might want to combine them since they look so similar ..

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48126/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48126/

[SparkQA]

Test build #143614 has finished for PR 34087 at commit 334ce1f.

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

[cloud-fan]

I assume this class is just copied from the original code file.

[tgravescs]

Should this be Developerapi as well? seems like it should match the ColumnarBatch

[flyrain]

@cloud-fan, exactly.
@tgravescs, I'm OK with both. Will make it a Developer api to be consistent.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48164/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48164/

[dbtsai]

LGTM too.

[dbtsai]

Merged into master. Thanks!

[SparkQA]

Test build #143651 has finished for PR 34087 at commit eac1012.

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