kunwp1 commented on code in PR #4086:
URL: https://github.com/apache/texera/pull/4086#discussion_r2578435797
##########
amber/src/main/scala/org/apache/texera/web/service/ExecutionResultService.scala:
##########
@@ -433,12 +433,27 @@ class ExecutionResultService(
storageUriOption match {
case Some(storageUri) =>
+ val (document, schemaOption) = DocumentFactory.openDocument(storageUri)
+ val virtualDocument = document.asInstanceOf[VirtualDocument[Tuple]]
+
+ val columns = if (request.columnLimit < Int.MaxValue &&
schemaOption.isDefined) {
Review Comment:
I think we don't need this if condition. Can you review the code to see if
this condition is always true?
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html:
##########
@@ -25,6 +25,39 @@ <h4>Empty result set</h4>
<div
[hidden]="!currentColumns"
class="result-table">
+ <div
+ class="column-navigation"
+ style="margin-bottom: 8px; display: flex; justify-content: flex-end; gap:
8px">
+ <button
+ nz-button
+ nzType="default"
+ (click)="onColumnShiftLeft()"
+ [disabled]="currentColumnOffset === 0">
+ <i
+ nz-icon
+ nzType="left"></i>
+ Previous Columns
+ </button>
+ <button
+ nz-button
+ nzType="default"
+ (click)="onColumnShiftRight()"
+ [disabled]="!currentColumns || currentColumns.length < columnLimit">
+ Next Columns
+ <i
+ nz-icon
+ nzType="right"></i>
+ </button>
+ </div>
+ <div
+ class="column-search"
+ style="margin-bottom: 8px; display: flex; justify-content: flex-end">
Review Comment:
ditto
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html:
##########
@@ -25,6 +25,39 @@ <h4>Empty result set</h4>
<div
[hidden]="!currentColumns"
class="result-table">
+ <div
+ class="column-navigation"
+ style="margin-bottom: 8px; display: flex; justify-content: flex-end; gap:
8px">
Review Comment:
Can you add a scss file instead of using inline `style`?
##########
common/workflow-operator/src/main/scala/org/apache/amber/operator/source/scan/csv/CSVScanSourceOpExec.scala:
##########
@@ -92,6 +92,7 @@ class CSVScanSourceOpExec private[csv] (descString: String)
extends SourceOperat
csvSetting.setMaxCharsPerColumn(-1)
csvSetting.setFormat(csvFormat)
csvSetting.setHeaderExtractionEnabled(desc.hasHeader)
+ csvSetting.setMaxColumns(10000)
Review Comment:
ditto
##########
common/workflow-operator/src/main/scala/org/apache/amber/operator/source/scan/csv/CSVScanSourceOpDesc.scala:
##########
@@ -92,6 +92,7 @@ class CSVScanSourceOpDesc extends ScanSourceOpDesc {
csvSetting.setFormat(csvFormat)
csvSetting.setHeaderExtractionEnabled(hasHeader)
csvSetting.setNullValue("")
+ csvSetting.setMaxColumns(10000)
Review Comment:
Can you make this magic number a configurable variable?
##########
common/workflow-core/src/main/scala/org/apache/amber/core/storage/result/iceberg/IcebergDocument.scala:
##########
@@ -150,8 +157,13 @@ private[storage] class IcebergDocument[T >: Null <:
AnyRef](
*
* @param from start from which record inclusively, if 0 means start from
the first
* @param until end at which record exclusively, if None means read to the
table's EOF
+ * @param columns columns to be projected
*/
- private def getUsingFileSequenceOrder(from: Int, until: Option[Int]):
Iterator[T] =
+ private def getUsingFileSequenceOrder(
+ from: Int,
+ until: Option[Int],
+ columns: Option[Seq[String]]
Review Comment:
Use a default value of `columns` to be `None` and revert all the changes you
made to add `None` in the arguments.
##########
common/workflow-core/src/main/scala/org/apache/amber/core/storage/model/VirtualDocument.scala:
##########
@@ -60,6 +60,16 @@ abstract class VirtualDocument[T] extends
ReadonlyVirtualDocument[T] {
def getRange(from: Int, until: Int): Iterator[T] =
throw new NotImplementedError("getRange method is not implemented")
+ /**
+ * get an iterator of a sequence starting from index `from`, until index
`until`
+ * @param from the starting index (inclusive)
+ * @param until the ending index (exclusive)
+ * @param columns the columns to be projected
+ * @return an iterator that returns data items of type T
+ */
+ def getRange(from: Int, until: Int, columns: Option[Seq[String]]):
Iterator[T] =
Review Comment:
You can merge two getRange() functions as a single function.
##########
common/workflow-core/src/main/scala/org/apache/amber/core/storage/result/iceberg/IcebergDocument.scala:
##########
@@ -100,20 +100,27 @@ private[storage] class IcebergDocument[T >: Null <:
AnyRef](
/**
* Get an iterator for reading records from the table.
*/
- override def get(): Iterator[T] = getUsingFileSequenceOrder(0, None)
+ override def get(): Iterator[T] = getUsingFileSequenceOrder(0, None, None)
/**
* Get records within a specified range [from, until).
*/
override def getRange(from: Int, until: Int): Iterator[T] = {
- getUsingFileSequenceOrder(from, Some(until))
+ getUsingFileSequenceOrder(from, Some(until), None)
+ }
+
+ /**
+ * Get records within a specified range [from, until) with specific columns.
+ */
+ override def getRange(from: Int, until: Int, columns: Option[Seq[String]]):
Iterator[T] = {
+ getUsingFileSequenceOrder(from, Some(until), columns)
Review Comment:
You can merge these functions together using default parameter.
##########
amber/src/main/scala/org/apache/texera/web/model/websocket/request/ResultPaginationRequest.scala:
##########
@@ -23,5 +23,8 @@ case class ResultPaginationRequest(
requestID: String,
operatorID: String,
pageIndex: Int,
- pageSize: Int
+ pageSize: Int,
+ columnOffset: Int = 0,
+ columnLimit: Int = Int.MaxValue,
Review Comment:
I see that the value of `columnLimit` is fixed to 25. If it's fixed this
way, I don't think we even need this variable in the frontend codebase. Can you
check?
--
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]
