Improve implementation of storage of transpiled files #1409
+305
−470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ile' into feature/multiplexer/store-transpiled-files * feature/specify-error-file/configure-transpile-errors-file: simplify and format write errors to tmp file add error-file flag to CLI and config # Conflicts: # src/databricks/labs/remorph/transpiler/execute.py
ericvergnaud
commented
Jan 7, 2025
| files: list[Path], | ||
| ) -> tuple[int, list[TranspileError]]: | ||
|
|
||
| output_folder = config.output_folder |
There was a problem hiding this comment.
moved this logic upwards
ericvergnaud
commented
Jan 7, 2025
| output_folder = input_source.parent / "transpiled" | ||
| make_dir(output_folder) | ||
| for source_dir, _, files in dir_walk(input_source): | ||
| relative_path = cast(Path, source_dir).relative_to(input_source) |
There was a problem hiding this comment.
'relative_path' clarifies the intent
ericvergnaud
commented
Jan 7, 2025
| return TranspileStatus([config.input_path], no_of_sqls, error_list) | ||
|
|
||
|
|
||
| @timeit | ||
| async def transpile( | ||
| workspace_client: WorkspaceClient, engine: TranspileEngine, config: TranspileConfig | ||
| ) -> tuple[list[dict[str, Any]], list[TranspileError]]: | ||
| ) -> tuple[dict[str, Any], list[TranspileError]]: |
There was a problem hiding this comment.
return a dict, we never have less or more than 1 status item
ericvergnaud
commented
Jan 7, 2025
| @@ -153,7 +146,7 @@ async def transpile( | |||
|
|
|||
| async def _do_transpile( | |||
| workspace_client: WorkspaceClient, engine: TranspileEngine, config: TranspileConfig | |||
| ) -> tuple[list[dict[str, Any]], list[TranspileError]]: | |||
| ) -> tuple[dict[str, Any], list[TranspileError]]: | |||
There was a problem hiding this comment.
return a dict, we never have less or more than 1 status item
ericvergnaud
commented
Jan 7, 2025
| status = { | ||
| "total_files_processed": len(result.file_list), | ||
| "total_queries_processed": result.no_of_transpiled_queries, | ||
| "analysis_error_count": result.analysis_error_count, |
ericvergnaud
commented
Jan 7, 2025
| @@ -80,7 +80,7 @@ async def transpile( | |||
| read_dialect = get_dialect(source_dialect) | |||
| error: TranspileError | None = self._check_supported(read_dialect, source_code, file_path) | |||
| if error: | |||
| return TranspileResult(str(file_path), 1, [error]) | |||
| return TranspileResult(source_code, 1, [error]) | |||
ericvergnaud
commented
Jan 7, 2025
tests/unit/test_cli_transpile.py
Outdated
| skip_validation, | ||
| catalog_name, | ||
| schema_name, | ||
| mode, | ||
| ) | ||
|
|
||
|
|
||
| def test_transpile_prints_errors(capsys, tmp_path, mock_workspace_client_cli): | ||
| def test_transpile_prints_errors(capsys, output_folder, error_file, mock_workspace_client_cli): |
There was a problem hiding this comment.
use fixtures to deduplicate test code and guarantee cleanup
ericvergnaud
commented
Jan 7, 2025
| @@ -31,260 +31,111 @@ def transpile(workspace_client: WorkspaceClient, engine: SqlglotEngine, config: | |||
| return asyncio.run(do_transpile(workspace_client, engine, config)) | |||
|
|
|||
|
|
|||
| def safe_remove_dir(dir_path: Path): | |||
There was a problem hiding this comment.
moved to conftest
ericvergnaud
commented
Jan 7, 2025
| return input_dir | ||
|
|
||
|
|
||
| def test_with_dir_skip_validation(initial_setup, mock_workspace_client): |
There was a problem hiding this comment.
removed duplicate test
ericvergnaud
commented
Jan 7, 2025
| assert ( | ||
| expected_message in actual_message | ||
| ), f"Message {actual_message} does not match the expected value {expected_message}" | ||
| assert match_count == len(expected_errors), "Not all expected errors were found" |
There was a problem hiding this comment.
fix missing assert when checking errors
src/databricks/labs/remorph/cli.py
Outdated
| @@ -35,12 +35,14 @@ def raise_validation_exception(msg: str) -> Exception: | |||
|
|
|||
|
|
|||
| @remorph.command | |||
| # pylint: disable=too-many-arguments | |||
There was a problem hiding this comment.
Better to increase the max-args in pyproject.toml than setting this at file level.
…d-files * bump-max-args: bump max args
ericvergnaud
added
stacked PR
…-lsp * bump-max-args: bump max args
…ultiplexer/refactor-transpile-error * feature/multiplexer/transpile-using-lsp: fix typo add docs bump max args
…ure/multiplexer/display-lsp-diagnostics-in-console * feature/multiplexer/refactor-transpile-error: fix typo add docs bump max args
… into feature/multiplexer/store-transpiled-files * feature/multiplexer/display-lsp-diagnostics-in-console: fix typo add docs
Base automatically changed from
feature/multiplexer/display-lsp-diagnostics-in-console
to
main
January 22, 2025 09:52
* main: Display lsp diagnostics in console (#1406) Refactor transpile error for LSP (#1405) Rename mock (#1423) Update databricks-sdk requirement from ~=0.29.0 to >=0.29,<0.42 (#1422) Transpile using LSP (#1404) Bump sqlglot from 26.0.1 to 26.1.3 (#1416) Configure transpile errors file (#1408) restore lost file and fix gitignore that led to that loss (#1414) bump max args (#1410) # Conflicts: # README.md # labs.yml # src/databricks/labs/remorph/cli.py # src/databricks/labs/remorph/config.py # src/databricks/labs/remorph/install.py # src/databricks/labs/remorph/transpiler/execute.py # src/databricks/labs/remorph/transpiler/lsp/lsp_engine.py # tests/unit/test_cli_transpile.py # tests/unit/test_install.py # tests/unit/transpiler/test_execute.py # tests/unit/transpiler/test_lsp_engine.py
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our current implementation already stores transpiled files.
However, there is no testing.
Also the corresponding business logic is spread across functions with similar names.
This PR:
Requires #1405 and #1406
Fixes #1302
Supersedes #1392