Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes CLI hanging on invalid queries #1274

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Fixes CLI hanging on invalid queries #1274

merged 6 commits into from
Dec 1, 2023

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Nov 30, 2023

Relevant Issues

Description

  • There was a bug introduced in our implementation of the CLI's RunnableWriter and RunnablePipeline.
    • To break it down:
      1. The main thread was designed to send interrupt signals to the relevant RunnableWriter and RunnablePipeline which would result in the tear-down of the child thread. We would then re-create the child thread and instantiate a new runnable in its place to continue to allow user inputs.
      2. There was a bug when the writer or pipeline were to fail for reasons beyond thread interruption. Essentially, on invalid queries, the child thread would die (due to unhandled exceptions), and the main thread would not know of its passing. So, the CLI would "hang" -- since we wouldn't be able to pass data between the runnables via the blocking queues.
  • This PR fixes this bug by handling invalid queries.

Reviewer Testing

  • I personally recommend running the following queries:
-- Regular query
SELECT a, b
FROM << { 'a': 1, 'b': 2 } >> AS t;

-- Regular query that should fail during eval
SELECT a, b, c, d
FROM << { 'a': 1, 'b': 2 } >> AS t;

-- Regular query that should fail during parse
SELECT a;

-- This adds to the global bindings
!add_to_global_env { 'a': 1 }

-- This should work
a;

-- This should hang on compilation. Use CTRL-C to escape.
SELECT COUNT(*)
FROM
  ([1, 2, 3, 4]) as x1,
  ([1, 2, 3, 4]) as x2,
  ([1, 2, 3, 4]) as x3,
  ([1, 2, 3, 4]) as x4,
  ([1, 2, 3, 4]) as x5,
  ([1, 2, 3, 4]) as x6,
  ([1, 2, 3, 4]) as x7,
  ([1, 2, 3, 4]) as x8,
  ([1, 2, 3, 4]) as x9,
  ([1, 2, 3, 4]) as x10,
  ([1, 2, 3, 4]) as x11
GROUP BY x1._1;

-- This should essentially run for a very long time during evaluation. Use CTRL-C to cancel.
SELECT *
FROM
  ([1, 2, 3, 4]) as x1,
  ([1, 2, 3, 4]) as x2,
  ([1, 2, 3, 4]) as x3,
  ([1, 2, 3, 4]) as x4,
  ([1, 2, 3, 4]) as x5,
  ([1, 2, 3, 4]) as x6,
  ([1, 2, 3, 4]) as x7,
  ([1, 2, 3, 4]) as x8,
  ([1, 2, 3, 4]) as x9,
  ([1, 2, 3, 4]) as x10,
  ([1, 2, 3, 4]) as x11;

-- If you start up the shell with --pipeline EXPERIMENTAL, you can run this:
EXPLAIN SELECT 1 FROM <<0, 1, 2>>

Other Information

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@johnedquinn johnedquinn changed the title Fixes CLI Fixes CLI hanging on invalid queries Nov 30, 2023
@johnedquinn johnedquinn requested a review from yliuuuu November 30, 2023 22:20
Copy link

github-actions bot commented Nov 30, 2023

Conformance comparison report

Base (7e3625f) 5916d8b +/-
% Passing 92.33% 92.33% 0.00%
✅ Passing 5372 5372 0
❌ Failing 446 446 0
🔶 Ignored 0 0 0
Total Tests 5818 5818 0

Number passing in both: 5372

Number failing in both: 446

Number passing in Base (7e3625f) but now fail: 0

Number failing in Base (7e3625f) but now pass: 0

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2023

Codecov Report

Attention: 72 lines in your changes are missing coverage. Please review.

Comparison is base (7e3625f) 71.57% compared to head (dff43e8) 71.60%.

Files Patch % Lines
...cli/src/main/kotlin/org/partiql/cli/shell/Shell.kt 0.00% 35 Missing ⚠️
...ain/kotlin/org/partiql/cli/shell/RunnableWriter.kt 0.00% 29 Missing ⚠️
...n/kotlin/org/partiql/cli/shell/RunnablePipeline.kt 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1274      +/-   ##
============================================
+ Coverage     71.57%   71.60%   +0.02%     
  Complexity     2451     2451              
============================================
  Files           229      229              
  Lines         17626    17620       -6     
  Branches       3249     3252       +3     
============================================
  Hits          12616    12616              
+ Misses         4001     3995       -6     
  Partials       1009     1009              
Flag Coverage Δ
CLI 13.74% <0.00%> (+0.04%) ⬆️
EXAMPLES 80.28% <ø> (ø)
LANG 78.71% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@yliuuuu yliuuuu left a comment

Choose a reason for hiding this comment

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

I can verify the fix works.

I feel like the state management is a bit tricky to understand.

Maybe we can think about if there is a better way to model this later.

@johnedquinn johnedquinn merged commit 6f6eb70 into main Dec 1, 2023
10 checks passed
@johnedquinn johnedquinn deleted the fix-interrupt branch December 1, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v0.13.1 CLI hangs on invalid queries
3 participants