Skip to content

feat: add artifacts-based configuration for selective destination syncing#126

Open
PuneetPunamiya wants to merge 3 commits into
redhat-data-and-ai:mainfrom
PuneetPunamiya:adds-support-for-selective-destination-sync
Open

feat: add artifacts-based configuration for selective destination syncing#126
PuneetPunamiya wants to merge 3 commits into
redhat-data-and-ai:mainfrom
PuneetPunamiya:adds-support-for-selective-destination-sync

Conversation

@PuneetPunamiya

@PuneetPunamiya PuneetPunamiya commented Mar 30, 2026

Copy link
Copy Markdown
Contributor
  • Enables selective syncing of file types with custom artifact paths under
    a stages directory structure

  • Users can configure which file types (converted/chunks/embeddings) to sync
    and customize their destination paths

  • Example usage in UnstructuredDataProduct

spec:
  destinationConfig:
    artifacts:
     - type: stage
       name: documentProcessorConfig
       path: processed-documents
     - type: stage
       name: chunksGeneratorConfig
       path: chunks
     - type: stage
       name: vectorEmbeddingsGeneratorConfig
       path: vector-embeddings

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a SyncToDestination configuration across the DocumentProcessor, ChunksGenerator, and VectorEmbeddingsGenerator components, allowing for granular control over which processed files are synchronized to the final destination. The controllers have been updated to conditionally trigger reconciliation and synchronization based on these flags and the presence of unprocessed files. Additionally, the Snowflake destination logic was refactored to handle multiple file types and prevent redundant uploads by comparing metadata. Review feedback suggests improving the robustness of JSON parsing for raw file paths by using minimal anonymous structs and refactoring the metadata comparison logic to reduce code duplication using generics.

Comment thread pkg/unstructured/destination.go Outdated
Comment thread pkg/unstructured/destination.go Outdated
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from ab1e868 to 973922f Compare March 31, 2026 10:00
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 973922f to 3d60885 Compare April 7, 2026 05:32
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 3d60885 to 847640c Compare April 7, 2026 05:39
@PuneetPunamiya PuneetPunamiya changed the title feat: add syncToDestination flags for selective file syncing feat: add artifacts-based configuration for selective destination syncing Apr 7, 2026
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 847640c to e1b8e69 Compare April 7, 2026 06:21
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from e1b8e69 to ea88c03 Compare April 7, 2026 08:10
Comment thread internal/controller/unstructureddataproduct_controller.go Outdated
Comment thread internal/controller/unstructureddataproduct_controller.go Outdated
Comment thread internal/controller/unstructureddataproduct_controller.go Outdated
Comment thread pkg/unstructured/destination.go Outdated
Comment thread pkg/unstructured/destination.go Outdated
Comment thread internal/controller/documentprocessor_controller.go Outdated
Comment thread internal/controller/chunksgenerator_controller.go Outdated
Comment thread internal/controller/unstructureddataproduct_controller.go Outdated
Comment thread internal/controller/unstructureddataproduct_controller.go Outdated
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from ea88c03 to b1341f3 Compare April 22, 2026 06:08
@piyush-garg piyush-garg modified the milestones: v0.6.0, v0.7.0 Apr 23, 2026
Comment thread api/v1alpha1/unstructureddatapipeline_types.go Outdated
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 84d60e0 to 32d667c Compare May 26, 2026 10:26
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 32d667c to 365fd2a Compare May 26, 2026 10:30
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 365fd2a to af29a89 Compare May 26, 2026 10:32
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from af29a89 to b7c3a73 Compare May 26, 2026 10:35
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from b7c3a73 to 5b1745a Compare May 28, 2026 07:06
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 5b1745a to 139536c Compare May 28, 2026 10:38
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 139536c to 5355fe1 Compare May 28, 2026 10:58

func buildDestinationPath(ctx context.Context, unstructuredDataProductCR operatorv1alpha1.UnstructuredDataPipeline, filePaths []string) []unstructured.ArtifactFiles {
logger := log.FromContext(ctx)
var artifactFileGroups []unstructured.ArtifactFiles

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is artifactFileGroup? is it not containing list of files ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here ArtifactFileGroup is one configured artifact i.e.

  • Files - list of file paths in filestore
  • Path - destination path

Shall I rename it to something like ArtifactSyncGroup


// list all files in the filestore for the data product
// list files to check if processing is needed
filePaths, err := r.fileStore.ListFilesInPath(ctx, dataProductName)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it is done here ?

}

artifactPath := artifact.Path
if artifactPath == "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can we check suffix and path together in single loop ?

Comment thread pkg/unstructured/destination.go Outdated
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 5355fe1 to 0878653 Compare June 2, 2026 13:41
…cing

Enables selective syncing of file types with custom artifact paths under
a stages directory structure

Users can configure which file types (converted/chunks/embeddings) to sync
and customize their destination paths

Example usage in UnstructuredDataProduct spec:
  destinationConfig:
    artifacts:
     - type: stage
       name: documentProcessorConfig
       path: processed-documents
     - type: stage
       name: chunksGeneratorConfig
       path: chunks
     - type: stage
       name: vectorEmbeddingsGeneratorConfig
       path: vector-embeddings

Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>
Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>
@PuneetPunamiya PuneetPunamiya force-pushed the adds-support-for-selective-destination-sync branch from 0878653 to aa1e161 Compare June 2, 2026 13:45
Signed-off-by: Puneet Punamiya <ppunamiy@redhat.com>
Comment thread pkg/unstructured/destination.go
Comment thread pkg/unstructured/destination.go
Comment thread test/e2e/unstructured_test.go
Comment on lines +198 to +202
var extractedPrefix string
if idx := strings.LastIndex(filePathInFilestore, "/"); idx != -1 {
extractedPrefix = filePathInFilestore[:idx]
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we are extracting out the prefix using last '/'index , what if there is '/' in the file name ??????
can't we take dp name and prefix from CR and create the complete prefix???

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants