Copilot commented on code in PR #6288:
URL: https://github.com/apache/texera/pull/6288#discussion_r3552941108
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csv/CSVScanSourceOpDesc.scala:
##########
@@ -78,10 +78,20 @@ class CSVScanSourceOpDesc extends ScanSourceOpDesc {
}
override def sourceSchema(): Schema = {
- if (customDelimiter.isEmpty || !fileResolved()) {
- return null
+ require(customDelimiter.isDefined, "Please specify a delimiter in the
properties panel.")
+ require(
+ fileResolved(),
+ "No file selected. Please select a valid .csv file from the 'File'
dropdown in the right panel."
+ )
+
Review Comment:
`customDelimiter` already has a default (","), and `getPhysicalOp` fills it
in when missing. Failing schema inference with "Please specify a delimiter" can
surface a misleading warning when the user hasn’t touched the delimiter field.
Prefer filling the default in `sourceSchema()` (same as `getPhysicalOp`) rather
than requiring the user to set it explicitly.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/sql/SQLSourceOpDesc.scala:
##########
@@ -138,12 +138,30 @@ abstract class SQLSourceOpDesc extends
SourceOperatorDescriptor {
* @return Schema
*/
private def querySchema: Schema = {
- if (
- this.host == null || this.port == null || this.database == null
- || this.table == null || this.username == null || this.password == null
- ) {
- return null
- }
+ require(
+ host != null && host.trim.nonEmpty,
+ s"Please enter a valid host name for the database in the properties
panel."
+ )
+ require(
+ port != null && port.trim.nonEmpty,
+ s"Please enter a valid port for the database in the properties panel."
+ )
+ require(
+ database != null && database.trim.nonEmpty,
+ s"Please enter a valid database name in the properties panel."
+ )
+ require(
+ table != null && table.trim.nonEmpty,
+ s"Please enter a valid table name in the properties panel."
+ )
+ require(
+ username != null && username.trim.nonEmpty,
+ s"Please enter a valid username in the properties panel."
+ )
+ require(
+ password != null && password.trim.nonEmpty,
+ s"Please enter a valid password in the properties panel."
+ )
Review Comment:
Requiring `password.trim.nonEmpty` is a behavior change from the previous
`null`-only check and will reject valid DB connections that use an empty
password (DriverManager accepts empty strings). If empty passwords should
remain allowed, only guard against `null` here.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/arrow/ArrowSourceOpDesc.scala:
##########
@@ -81,8 +94,11 @@ class ArrowSourceOpDesc extends ScanSourceOpDesc {
val arrowSchema: ArrowSchema = reader.getVectorSchemaRoot.getSchema
ArrowUtils.toTexeraSchema(arrowSchema)
}
- .getOrElse {
- throw new IOException("Failed to infer schema from Arrow file.")
- }
+ .recover { case e: Throwable =>
+ throw new RuntimeException(
+ "Failed to read the .arrow file. Please ensure it is a valid Arrow
file.",
+ e
+ )
+ }.get
Review Comment:
Catching `Throwable` is too broad (it can include fatal errors like
`VirtualMachineError`). Even though `Using`/`Try` typically only captures
non-fatal exceptions, it’s better to be explicit here and only wrap `NonFatal`
failures.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csv/ParallelCSVScanSourceOpDesc.scala:
##########
@@ -79,10 +79,20 @@ class ParallelCSVScanSourceOpDesc extends ScanSourceOpDesc {
}
override def sourceSchema(): Schema = {
- if (customDelimiter.isEmpty || !fileResolved()) {
- return null
+ require(customDelimiter.isDefined, "Please specify a delimiter in the
properties panel.")
+ require(
+ fileResolved(),
+ "No file selected. Please select a valid .csv file from the 'File'
dropdown in the right panel."
+ )
+
Review Comment:
`customDelimiter` is defaulted to "," in `getPhysicalOp` when missing.
Requiring it in `sourceSchema()` can produce a misleading validation error when
the delimiter was simply left at its default. Consider filling the default in
`sourceSchema()` (same logic as `getPhysicalOp`) and only requiring the file
selection.
##########
frontend/src/app/workspace/component/result-panel/error-frame/error-frame.component.ts:
##########
@@ -72,11 +72,41 @@ export class ErrorFrameComponent implements OnInit {
errorMessages = errorMessages.filter(err => err.operatorId ===
this.operatorId);
}
this.categoryToErrorMapping = errorMessages.reduce((acc, obj) => {
- const key = obj.type.name;
+ let key = obj.type.name;
+ let message = obj.message;
+ let details = obj.details;
+
+ if (key === "COMPILATION_ERROR") {
+ key = "WARNING";
Review Comment:
`COMPILATION_ERROR` entries are being re-labeled to the category key
`WARNING`. This appears to go beyond the PR description (“strip internal Java
prefixes”) and can also be misleading since these errors correspond to
`CompilationState.Failed` (compilation didn’t succeed). Consider keeping the
category as `COMPILATION_ERROR` and only formatting the message/details, or
introduce a dedicated UI label mapping instead of changing the underlying
category key.
--
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]
