Copilot commented on code in PR #4364: URL: https://github.com/apache/texera/pull/4364#discussion_r3067214930
########## frontend/src/app/workspace/component/dataset-version-selector/dataset-version-selector.component.ts: ########## @@ -0,0 +1,85 @@ +/** + * 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. + */ + +import { ChangeDetectorRef, Component, OnInit } from "@angular/core"; +import { FieldType, FieldTypeConfig } from "@ngx-formly/core"; +import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; +import { DashboardDataset } from "../../../dashboard/type/dashboard-dataset.interface"; +import { DatasetVersion } from "../../../common/type/dataset"; +import { DatasetService } from "../../../dashboard/service/user/dataset/dataset.service"; + +@UntilDestroy() +@Component({ + selector: "texera-dataset-version-selector-template", + templateUrl: "./dataset-version-selector.component.html", +}) +export class DatasetVersionSelectorComponent extends FieldType<FieldTypeConfig> implements OnInit { + datasets: ReadonlyArray<DashboardDataset> = []; + datasetVersions: ReadonlyArray<DatasetVersion> = []; + selectedDataset?: DashboardDataset; + selectedVersion?: DatasetVersion; + + constructor( + private datasetService: DatasetService, + private changeDetectorRef: ChangeDetectorRef + ) { + super(); + } + + ngOnInit(): void { + this.datasetService + .retrieveAccessibleDatasets() + .pipe(untilDestroyed(this)) + .subscribe(datasets => { + this.datasets = datasets; + const [_, ownerEmail, datasetName, versionName] = this.formControl.value.split("/"); + if (versionName) { + this.selectedDataset = this.datasets.find( + dataset => dataset.ownerEmail === ownerEmail && dataset.dataset.name === datasetName + ); Review Comment: this.formControl.value can be null/undefined on initial render (especially since the field can be cleared). Calling split("/") unconditionally will throw and break the property panel. Guard by checking that the value is a non-empty string before splitting, and handle malformed paths defensively. ########## frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts: ########## @@ -453,6 +453,10 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On mappedField.type = "inputautocomplete"; } + if (mappedField.key == "datasetVersionPath") { + mappedField.type = "datasetversionselector"; + } Review Comment: Use strict equality for mappedField.key comparisons ("===") to match the rest of this file (e.g., dummyOperator/dummyProperty checks) and avoid unexpected coercion when key isn’t a string. Consider updating both the existing fileName check and the new datasetVersionPath check for consistency. ########## common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/DatasetSelectorSourceOpExec.scala: ########## @@ -0,0 +1,60 @@ +/* + * 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. + */ + +package org.apache.texera.amber.operator.source.dataset + +import org.apache.texera.amber.core.executor.SourceOperatorExecutor +import org.apache.texera.amber.core.storage.util.LakeFSStorageClient +import org.apache.texera.amber.core.tuple.TupleLike +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.apache.texera.dao.SqlServer +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 + +class DatasetSelectorSourceOpExec private[dataset] (descString: String) + extends SourceOperatorExecutor { + private val desc: DatasetSelectorSourceOpDesc = + objectMapper.readValue(descString, classOf[DatasetSelectorSourceOpDesc]) + + override def produceTuple(): Iterator[TupleLike] = { + val Seq(_, ownerEmail, datasetName, versionName) = + desc.datasetVersionPath.split("/").toSeq + Review Comment: There’s no automated coverage for DatasetSelectorSourceOpExec’s core behaviors (path parsing, DB lookup failure handling, output path formatting). Given similar source executors have unit tests, consider adding tests that at least verify parsing/validation behavior and the error message when dataset/version cannot be resolved (mocking LakeFS/DB or factoring parsing into a pure helper). ########## frontend/src/app/workspace/component/dataset-version-selector/dataset-version-selector.component.ts: ########## @@ -0,0 +1,85 @@ +/** + * 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. + */ + +import { ChangeDetectorRef, Component, OnInit } from "@angular/core"; +import { FieldType, FieldTypeConfig } from "@ngx-formly/core"; +import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; +import { DashboardDataset } from "../../../dashboard/type/dashboard-dataset.interface"; +import { DatasetVersion } from "../../../common/type/dataset"; +import { DatasetService } from "../../../dashboard/service/user/dataset/dataset.service"; + +@UntilDestroy() +@Component({ + selector: "texera-dataset-version-selector-template", + templateUrl: "./dataset-version-selector.component.html", +}) +export class DatasetVersionSelectorComponent extends FieldType<FieldTypeConfig> implements OnInit { + datasets: ReadonlyArray<DashboardDataset> = []; + datasetVersions: ReadonlyArray<DatasetVersion> = []; + selectedDataset?: DashboardDataset; + selectedVersion?: DatasetVersion; + + constructor( + private datasetService: DatasetService, + private changeDetectorRef: ChangeDetectorRef + ) { + super(); + } + + ngOnInit(): void { + this.datasetService + .retrieveAccessibleDatasets() + .pipe(untilDestroyed(this)) + .subscribe(datasets => { + this.datasets = datasets; + const [_, ownerEmail, datasetName, versionName] = this.formControl.value.split("/"); + if (versionName) { + this.selectedDataset = this.datasets.find( + dataset => dataset.ownerEmail === ownerEmail && dataset.dataset.name === datasetName + ); + this.onDatasetChange(); + } + }); + } + + onDatasetChange(): void { + if (this.selectedDataset) { + this.datasetService + .retrieveDatasetVersionList(this.selectedDataset.dataset.did!) + .pipe(untilDestroyed(this)) + .subscribe(versions => { + this.datasetVersions = versions; + this.selectedVersion = versions[0]; Review Comment: When initializing from an existing datasetVersionPath, onDatasetChange() always selects versions[0], which overwrites the persisted versionName from the form control. Preserve the existing versionName by selecting the matching version from `versions` (and only default to versions[0] when there is no preselected version). ```suggestion const [_, __, ___, versionName] = (this.formControl.value ?? "").split("/"); this.selectedVersion = versionName ? versions.find(version => version.name === versionName) ?? versions[0] : versions[0]; ``` ########## common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/dataset/DatasetSelectorSourceOpExec.scala: ########## @@ -0,0 +1,60 @@ +/* + * 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. + */ + +package org.apache.texera.amber.operator.source.dataset + +import org.apache.texera.amber.core.executor.SourceOperatorExecutor +import org.apache.texera.amber.core.storage.util.LakeFSStorageClient +import org.apache.texera.amber.core.tuple.TupleLike +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.apache.texera.dao.SqlServer +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 + +class DatasetSelectorSourceOpExec private[dataset] (descString: String) + extends SourceOperatorExecutor { + private val desc: DatasetSelectorSourceOpDesc = + objectMapper.readValue(descString, classOf[DatasetSelectorSourceOpDesc]) + + override def produceTuple(): Iterator[TupleLike] = { + val Seq(_, ownerEmail, datasetName, versionName) = + desc.datasetVersionPath.split("/").toSeq + + val (repositoryName, versionHash) = + SqlServer + .getInstance() + .createDSLContext() + .select(DATASET.REPOSITORY_NAME, DATASET_VERSION.VERSION_HASH) + .from(DATASET) + .join(USER) + .on(USER.UID.eq(DATASET.OWNER_UID)) + .join(DATASET_VERSION) + .on(DATASET_VERSION.DID.eq(DATASET.DID)) + .where(USER.EMAIL.eq(ownerEmail)) + .and(DATASET.NAME.eq(datasetName)) + .and(DATASET_VERSION.NAME.eq(versionName)) + .fetchOne(r => (r.value1(), r.value2())) Review Comment: produceTuple assumes datasetVersionPath is non-null and exactly of the form /ownerEmail/datasetName/versionName; otherwise the Seq destructuring will throw. Also, fetchOne(...) returns null when no record matches, which will cause a MatchError/NPE during tuple destructuring. Validate/parse the path safely (e.g., filter empty segments and check arity) and handle missing DB rows by throwing a clear exception (or returning an empty iterator) before calling LakeFSStorageClient. ```suggestion private def parseDatasetVersionPath(datasetVersionPath: String): (String, String, String) = { val invalidPathMessage = "datasetVersionPath must be non-null and of the form /ownerEmail/datasetName/versionName" Option(datasetVersionPath) .map(_.split("/").toSeq.filter(_.nonEmpty)) .getOrElse(throw new IllegalArgumentException(invalidPathMessage)) match { case Seq(ownerEmail, datasetName, versionName) => (ownerEmail, datasetName, versionName) case _ => throw new IllegalArgumentException(invalidPathMessage) } } override def produceTuple(): Iterator[TupleLike] = { val (ownerEmail, datasetName, versionName) = parseDatasetVersionPath(desc.datasetVersionPath) val (repositoryName, versionHash) = Option( SqlServer .getInstance() .createDSLContext() .select(DATASET.REPOSITORY_NAME, DATASET_VERSION.VERSION_HASH) .from(DATASET) .join(USER) .on(USER.UID.eq(DATASET.OWNER_UID)) .join(DATASET_VERSION) .on(DATASET_VERSION.DID.eq(DATASET.DID)) .where(USER.EMAIL.eq(ownerEmail)) .and(DATASET.NAME.eq(datasetName)) .and(DATASET_VERSION.NAME.eq(versionName)) .fetchOne(r => (r.value1(), r.value2())) ).getOrElse( throw new NoSuchElementException( s"No dataset version found for path ${desc.datasetVersionPath}" ) ) ``` -- 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]
