Copilot commented on code in PR #6502:
URL: https://github.com/apache/texera/pull/6502#discussion_r3769712062
##########
common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/FileResolver.scala:
##########
@@ -88,23 +88,23 @@ object FileResolver {
val filePath = Paths.get(fileName)
val pathSegments = (0 until
filePath.getNameCount).map(filePath.getName(_).toString).toArray
- if (pathSegments.length < 4) {
+ if (pathSegments.length < 5 ||
!ResourceType.isValidPrefix(pathSegments(0))) {
Review Comment:
This prefix check runs only inside `datasetResolveFunc`, after `resolve` has
already tried `localResolveFunc`. Thus a logical `/datasets/...` path that also
exists on the worker filesystem resolves as a local file and never uses the
resource prefix to select the backing table, contrary to the namespacing
contract. Dispatch known resource prefixes before local-path probing (and add a
collision regression test).
##########
sql/updates/36.sql:
##########
@@ -0,0 +1,179 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+\c texera_db
+
+SET search_path TO texera_db;
+
+BEGIN;
+
+-- The file resolver now requires an explicit resource-type prefix on dataset
+-- logical paths (/datasets/ownerEmail/datasetName/versionName/...) so other
+-- resource types (e.g. models) can be told apart by the prefix. Existing
+-- workflows store unprefixed dataset paths inside workflow.content and
+-- workflow_version.content, in two operator properties:
+-- * fileName (scan-source operators): /owner/name/version/file
+-- * datasetVersionPath (file-lister operator): /owner/name/version
+-- This migration prepends the "datasets" segment to both.
+--
+-- A value is treated as a dataset path only when its first two segments match
an
+-- existing (user.email, dataset.name) pair -- that pair is unique.
+-- Local file paths and URLs match no dataset and are left untouched.
+-- Already-prefixed values are skipped (idempotent). jsonb_set
+-- uses create_missing = false so absent properties are never added.
+
+DO $$
+DECLARE
+ wf_count INT := 0;
+ wv_count INT := 0;
+BEGIN
+ WITH affected AS (
+ SELECT w.wid
+ FROM workflow w,
+ jsonb_array_elements(w.content::jsonb -> 'operators') AS op,
Review Comment:
`jsonb_array_elements` is evaluated in the `FROM` clause before this query's
`jsonb_typeof(...) = 'array'` predicate can filter the row. Consequently, valid
JSON whose `operators` member is an object/string aborts the entire Liquibase
migration instead of being skipped. Feed the function an empty array for
non-array values (the later predicate may remain as a redundant guard).
This issue also appears on line 114 of the same file.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/FileListerSourceOpExec.scala:
##########
@@ -28,13 +29,34 @@ import
org.apache.texera.dao.jooq.generated.tables.Dataset.DATASET
import
org.apache.texera.dao.jooq.generated.tables.DatasetVersion.DATASET_VERSION
import org.apache.texera.dao.jooq.generated.tables.User.USER
+object FileListerSourceOpExec {
+
+ /**
+ * Parses a dataset version path
(/datasets/ownerEmail/datasetName/versionName) into its
+ * (ownerEmail, datasetName, versionName) components.
+ *
+ * @throws IllegalArgumentException if the path is not a well-formed
dataset version path
+ */
+ private[dataset] def parseDatasetVersionPath(
+ datasetVersionPath: String
+ ): (String, String, String) = {
+ val segments = datasetVersionPath.split("/").filter(_.nonEmpty)
+ require(
+ segments.length >= 4 && ResourceType.isValidPrefix(segments.head),
+ s"Invalid dataset version path '$datasetVersionPath'; " +
+ "expected /datasets/ownerEmail/datasetName/versionName"
+ )
+ (segments(1), segments(2), segments(3))
+ }
+}
+
class FileListerSourceOpExec private[dataset] (descString: String) extends
SourceOperatorExecutor {
private val desc: FileListerSourceOpDesc =
objectMapper.readValue(descString, classOf[FileListerSourceOpDesc])
override def produceTuple(): Iterator[TupleLike] = {
- val Seq(_, ownerEmail, datasetName, versionName, _*) =
- desc.datasetVersionPath.split("/").toSeq
+ val (ownerEmail, datasetName, versionName) =
+ FileListerSourceOpExec.parseDatasetVersionPath(desc.datasetVersionPath)
Review Comment:
The parser accepts and discards extra segments, but `produceTuple` later
prepends the unchanged `desc.datasetVersionPath` to every LakeFS object. For
the added `/datasets/alice/ds/v2/extra/` test case, it therefore emits paths
under `v2/extra/...`, which resolve to different/nonexistent objects. Either
require exactly four segments or construct emitted paths from the parsed
canonical prefix.
--
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]
