emkornfield commented on a change in pull request #10652:
URL: https://github.com/apache/arrow/pull/10652#discussion_r716975958
##########
File path:
java/dataset/src/main/java/org/apache/arrow/dataset/scanner/ScanOptions.java
##########
@@ -26,12 +26,16 @@
/**
* Constructor.
- * @param columns Projected columns. Empty for scanning all columns.
* @param batchSize Maximum row number of each returned {@link
org.apache.arrow.vector.ipc.message.ArrowRecordBatch}
+ * @param columns Projected columns. Null for scanning all columns.
*/
- public ScanOptions(String[] columns, long batchSize) {
- this.columns = columns;
+ public ScanOptions(long batchSize, String[] columns) {
Review comment:
By backwards compatibility I mean not breaking existing code written
again the API. In this case since it is easy being friendly to consumers makes
sense.
I'm suggesting mark this constructor as deprecated but don't change the
ordering of the fields. Update the implementation here using one of the new
constructors proposed (I prefer the first option). IIUC the update would be
passing optional.absent to the new constructor when columns.length == 0.
This approach allows anybody using the existing constructor to still
function and new users can use the constructor that takes the optional
parameter with the new semantics.
I'd wait on the builder pattern until we have to add more parameters but
ultimately that seems like a good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
