diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs index 7d559e55..d496142a 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularToolRunner.cs @@ -57,6 +57,23 @@ public static async Task PrepareAsync(IServiceProvider servic context.ImportToWorkspaceAsync, cancellationToken); + if (tables.Count == 0) + { + // The conversation has tabular files but none of them could be read. Say so instead of + // handing back an empty workspace, which reads to the model as files that contain no data. + workspace.Dispose(); + + logger?.LogWarning( + "None of the {DocumentCount} tabular document(s) attached to this conversation could be loaded into the workspace.", + context.Documents.Count); + + return new PreparationResult( + null, + null, + null, + "The tabular files attached to this conversation could not be read. Their stored content is unavailable, so no data can be queried. Ask the user to upload the files again."); + } + if (logger?.IsEnabled(LogLevel.Debug) == true) { logger.LogDebug( diff --git a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs index fc3e479b..8d404206 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents/Tabular/TabularWorkspace.cs @@ -120,6 +120,7 @@ public async Task> EnsureReadyAsync( try { + RemoveFailedImportPlaceholderTables(); RemoveTablesForDetachedDocuments(documents); await SynchronizeTablesAsync(documents, artifactLoader, workspaceImporter, cancellationToken); } @@ -593,7 +594,10 @@ private async Task SynchronizeTablesAsync( var importResults = await workspaceImporter(document, _connection, allocator, cancellationToken); - if (importResults != null) + // An importer that produced nothing falls through to the artifact loader rather than + // marking the document imported, so a failed streaming import still gets a second + // chance and never leaves the document represented by no table at all. + if (importResults is { Count: > 0 }) { foreach (var importResult in importResults) { @@ -616,7 +620,23 @@ private async Task SynchronizeTablesAsync( } var artifact = await artifactLoader(document, cancellationToken); - var worksheets = artifact?.GetWorksheets() ?? [new TabularWorksheet()]; + var worksheets = (artifact?.GetWorksheets() ?? []).Where(HasContent).ToList(); + + // A document whose content could not be loaded (a missing artifact, missing chunks, or an + // unreadable file) must not be turned into a table. Creating one would write a placeholder + // column with no rows, register it in the metadata, and make IsDocumentLoaded true forever + // after, so the workspace would serve an empty table for the rest of the conversation and + // never retry the import. Leaving it unregistered means the next request tries again. + if (worksheets.Count == 0) + { + _logger.LogWarning( + "No tabular content could be loaded for document '{FileName}' ('{DocumentId}'), so no table was created. The import will be retried on the next request.", + document.FileName, + document.DocumentId); + + continue; + } + var singleWorksheetDocument = worksheets.Count == 1; foreach (var worksheet in worksheets) @@ -721,6 +741,106 @@ private void DeleteMetadataEntry(string tableName) command.ExecuteNonQuery(); } + /// + /// Discards tables left behind by an import that produced no content, so the document is read + /// again on this request. + /// + /// + /// Such a table was created before a failed import stopped being registered, and it is indelible + /// on its own: the metadata entry makes the document count as loaded, so the workspace serves an + /// empty table for the rest of the conversation and never reads the file again. This repairs a + /// database already in that state without waiting for the user to upload the files a second time. + /// Must run inside a write window. + /// + private void RemoveFailedImportPlaceholderTables() + { + if (_tables.Count == 0) + { + return; + } + + List placeholders = null; + + foreach (var table in _tables.Values) + { + if (IsFailedImportPlaceholder(table.TableName)) + { + (placeholders ??= []).Add(table); + } + } + + if (placeholders is null) + { + return; + } + + foreach (var table in placeholders) + { + DropTable(table.TableName); + DeleteMetadataEntry(table.TableName); + _tables.Remove(table.TableName); + + _logger.LogWarning( + "Discarded the empty table '{TableName}' left behind by a failed import of document '{DocumentId}'. The document will be imported again.", + table.TableName, + table.DocumentId); + } + } + + /// + /// Determines whether a table has the exact shape a failed import leaves behind: the single + /// placeholder value column and no rows. + /// + /// + /// The row check is what makes this safe to act on. A table the user emptied through a + /// manipulation tool keeps the real columns of its source file, so it can never match, and + /// re-importing something that matches cannot lose data because it holds none. + /// + /// The table to inspect. + /// when the table is a failed import's leftover. + private bool IsFailedImportPlaceholder(string tableName) + { + using (var schemaCommand = _connection.CreateCommand()) + { + schemaCommand.CommandText = $"PRAGMA table_info({QuoteIdentifier(tableName)})"; + + using var reader = schemaCommand.ExecuteReader(); + + // Exactly one column, named "value". + if (!reader.Read() || !string.Equals(reader.GetString(1), "value", StringComparison.Ordinal) || reader.Read()) + { + return false; + } + } + + using var rowCommand = _connection.CreateCommand(); + rowCommand.CommandText = $"SELECT EXISTS(SELECT 1 FROM {QuoteIdentifier(tableName)})"; + + return Convert.ToInt64(rowCommand.ExecuteScalar()) == 0; + } + + /// + /// Determines whether a worksheet carries anything worth creating a table for. A worksheet with + /// no header and no rows means the import produced nothing, not that the spreadsheet is empty. + /// + /// The parsed worksheet. + /// when the worksheet has a header or rows. + private static bool HasContent(TabularWorksheet worksheet) + { + if (worksheet is null) + { + return false; + } + + if (worksheet.Rows is { Count: > 0 }) + { + return true; + } + + return worksheet.Header is { Count: > 0 } + && worksheet.Header.Any(name => !string.IsNullOrWhiteSpace(name)); + } + private bool IsDocumentLoaded(string documentId) { foreach (var table in _tables.Values) diff --git a/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs index bdaf7fb9..af9793cd 100644 --- a/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs +++ b/tests/CrestApps.Core.Tests/Core/Documents/Tabular/TabularWorkspaceLifecycleTests.cs @@ -229,6 +229,150 @@ await workspace.EnsureReadyAsync( } } + [Fact] + public async Task EnsureReadyAsync_WhenADocumentsContentCannotBeLoaded_CreatesNoTable() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using var workspace = CreateWorkspace(databasePath); + + // A placeholder table here would read to the model as a file that contains no data. + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "projections.xlsx")], + LoaderFor(), + cancellationToken); + + Assert.Empty(tables); + Assert.Empty(GetUserTableNames(databasePath)); + Assert.Empty(GetMetadataTableNames(databasePath)); + } + + [Fact] + public async Task EnsureReadyAsync_WhenContentBecomesAvailableLater_RetriesTheFailedImport() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using (var workspace = CreateWorkspace(databasePath)) + { + await workspace.EnsureReadyAsync([Document("doc-1", "sales.csv")], LoaderFor(), cancellationToken); + } + + // Registering the failed import would make IsDocumentLoaded true forever after, so the + // workspace would keep serving an empty table and never read the file again. + using (var workspace = CreateWorkspace(databasePath)) + { + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal("sales", table.TableName); + Assert.Equal(2, table.RowCount); + } + } + + [Fact] + public async Task EnsureReadyAsync_WhenOneDocumentFails_KeepsTheDocumentsThatLoaded() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using var workspace = CreateWorkspace(databasePath); + + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv"), Document("doc-2", "budget.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal("sales", table.TableName); + Assert.Equal(2, table.RowCount); + } + + [Fact] + public async Task EnsureReadyAsync_WhenTheDatabaseAlreadyHoldsAFailedImportsEmptyTable_ImportsTheDocumentAgain() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + // The shape an earlier build wrote when a document's content could not be loaded. The metadata + // entry made the document count as loaded, so the empty table was served forever after. + WritePlaceholderTable(databasePath, "sales", "doc-1", "sales.csv"); + + using var workspace = CreateWorkspace(databasePath); + + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal("sales", table.TableName); + Assert.Equal(2, table.RowCount); + Assert.Equal(["region", "amount"], table.Columns.Select(c => c.Name)); + } + + [Fact] + public async Task EnsureReadyAsync_DoesNotReimportATableTheUserEmptied() + { + var cancellationToken = TestContext.Current.CancellationToken; + var databasePath = DatabasePath("session-1"); + + using var workspace = CreateWorkspace(databasePath); + + await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + // Deleting every row through the manipulation tool is a deliberate edit, not a failed import. + await workspace.ExecuteAsync("DELETE FROM sales", cancellationToken); + + var tables = await workspace.EnsureReadyAsync( + [Document("doc-1", "sales.csv")], + LoaderFor(("doc-1", SalesCsv)), + cancellationToken); + + var table = Assert.Single(tables); + Assert.Equal(0, table.RowCount); + Assert.Equal(["region", "amount"], table.Columns.Select(c => c.Name)); + } + + private static void WritePlaceholderTable(string databasePath, string tableName, string documentId, string fileName) + { + using var connection = new SqliteConnection($"Data Source={databasePath};Pooling=False"); + connection.Open(); + + using (var schemaCommand = connection.CreateCommand()) + { + schemaCommand.CommandText = $""" + CREATE TABLE IF NOT EXISTS "_workspace_meta" ( + "table_name" TEXT PRIMARY KEY, + "document_id" TEXT NOT NULL, + "worksheet_name" TEXT, + "file_name" TEXT NOT NULL, + "source_names_json" TEXT NOT NULL + ); + CREATE TABLE "{tableName}" ("value" TEXT); + """; + schemaCommand.ExecuteNonQuery(); + } + + using var command = connection.CreateCommand(); + command.CommandText = """ + INSERT INTO "_workspace_meta" ("table_name", "document_id", "worksheet_name", "file_name", "source_names_json") + VALUES ($tableName, $documentId, NULL, $fileName, $sourceNames) + """; + command.Parameters.AddWithValue("$tableName", tableName); + command.Parameters.AddWithValue("$documentId", documentId); + command.Parameters.AddWithValue("$fileName", fileName); + command.Parameters.AddWithValue("$sourceNames", "{\"value\":null}"); + command.ExecuteNonQuery(); + } + private string DatabasePath(string referenceId) { var path = Path.Combine(_root, "documents", "chat-session", referenceId, "data", "tabular.db");