Copilot commented on code in PR #6288:
URL: https://github.com/apache/texera/pull/6288#discussion_r3550288053
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/json/JSONLScanSourceOpDesc.scala:
##########
@@ -68,10 +68,14 @@ class JSONLScanSourceOpDesc extends ScanSourceOpDesc {
}
override def sourceSchema(): Schema = {
- if (!fileResolved()) {
- return null
+ require(fileResolved(), "No file selected. Please select a valid .jsonl
file from the 'File' dropdown in the right panel.")
+
+ val stream = try {
+ DocumentFactory.openReadonlyDocument(new
URI(fileName.get)).asInputStream()
+ } catch {
+ case _: Exception =>
+ throw new RuntimeException("The selected item is a folder, not a file.
Please select an actual .jsonl file from the 'File' dropdown.")
}
Review Comment:
The broad catch here converts any failure to open the URI (missing file,
permission error, bad URI, etc.) into a "folder" message. Also, the rethrown
RuntimeException drops the original cause, making the backend 'Stack trace for
developers' less useful. Prefer an explicit directory/existence check for local
files and let unexpected IO failures surface with their original cause.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/sql/SQLSourceOpDesc.scala:
##########
@@ -138,12 +138,12 @@ 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, s"Please enter a valid password in the
properties panel.")
Review Comment:
Password validation only checks for null, so an empty/whitespace password
will still attempt a DB connection and can surface a cryptic SQL auth error.
Validate non-empty (trimmed) like the other fields so users get the intended
actionable message.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csvOld/CSVOldScanSourceOpDesc.scala:
##########
@@ -75,11 +75,16 @@ class CSVOldScanSourceOpDesc 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.")
+
// infer schema from the first few lines of the file
- val file = DocumentFactory.openReadonlyDocument(new
URI(fileName.get)).asFile()
+ val file = try {
+ DocumentFactory.openReadonlyDocument(new URI(fileName.get)).asFile()
+ } catch {
+ case _: Exception =>
+ throw new RuntimeException("The selected item is a folder, not a file.
Please select an actual .csv file from the 'File' dropdown.")
+ }
Review Comment:
This catch-all rethrows every failure as "folder, not a file", which is
inaccurate for missing files/permission errors and also discards the original
exception cause (reducing debuggability in compilation error details). Use an
explicit local file check (directory/existence) and avoid swallowing unrelated
IO errors.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csv/ParallelCSVScanSourceOpDesc.scala:
##########
@@ -79,10 +79,15 @@ 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.")
+
+ val file = try {
+ DocumentFactory.openReadonlyDocument(new URI(fileName.get)).asFile()
+ } catch {
+ case _: Exception =>
+ throw new RuntimeException("The selected item is a folder, not a file.
Please select an actual .csv file from the 'File' dropdown.")
}
Review Comment:
Catching all exceptions and always reporting "folder" can mislead users when
the file is missing or unreadable, and it drops the original cause (hurting the
developer stack trace). Prefer a deterministic local file check
(directory/existence) and let other failures bubble up.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/csv/CSVScanSourceOpDesc.scala:
##########
@@ -78,10 +78,15 @@ 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.")
+
+ val stream = try {
+ DocumentFactory.openReadonlyDocument(new
URI(fileName.get)).asInputStream()
+ } catch {
+ case _: Exception =>
+ throw new RuntimeException("The selected item is a folder, not a file.
Please select an actual .csv file from the 'File' dropdown.")
}
Review Comment:
This catch-all converts every open/read failure into a "folder" message
(missing file, permission error, etc.) and loses the original exception cause.
Use an explicit local file check (directory/existence) and allow other IO
failures to propagate with their real details.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/arrow/ArrowSourceOpDesc.scala:
##########
@@ -71,7 +71,15 @@ class ArrowSourceOpDesc extends ScanSourceOpDesc {
*/
@Override
def inferSchema(): Schema = {
- val file = DocumentFactory.openReadonlyDocument(new
URI(fileName.get)).asFile()
+ require(fileResolved(), "No file selected. Please select a valid .arrow
file from the 'File' dropdown in the right panel.")
+
+ val file = try {
+ DocumentFactory.openReadonlyDocument(new URI(fileName.get)).asFile()
+ } catch {
Review Comment:
Two issues: (1) the current catch-all assumes every open failure means the
selection is a folder, which is inaccurate for missing/unreadable files, and it
discards the original cause; (2) the Using.getOrElse branch throws a new
RuntimeException without attaching the underlying failure, losing the real
parsing/IO error in compilation details. Prefer an explicit local-file
directory/existence check and preserve the original Throwable as the cause when
wrapping.
--
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]
