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

Fixed the bug on CLI command with input argument #478

Merged
merged 2 commits into from
Nov 29, 2021
Merged

Fixed the bug on CLI command with input argument #478

merged 2 commits into from
Nov 29, 2021

Conversation

lziq
Copy link
Contributor

@lziq lziq commented Nov 23, 2021

Issue #475

Description of changes:

  1. Fixed the issue by assuming the input data is empty, if users do not explicit specify the input data argument while using CLI.
  2. Creates one test case to cover the case.

@lziq lziq self-assigned this Nov 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #478 (60242ec) into main (36e20d1) will increase coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #478      +/-   ##
============================================
+ Coverage     82.38%   82.41%   +0.02%     
- Complexity     1406     1407       +1     
============================================
  Files           171      171              
  Lines         10816    10817       +1     
  Branches       1781     1782       +1     
============================================
+ Hits           8911     8915       +4     
+ Misses         1361     1359       -2     
+ Partials        544      543       -1     
Flag Coverage Δ
CLI 20.33% <50.00%> (+1.26%) ⬆️
EXAMPLES 74.85% <ø> (ø)
LANG 84.85% <ø> (ø)
PTS ∅ <ø> (∅)
TEST_SCRIPT 79.68% <ø> (ø)

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

Impacted Files Coverage Δ
cli/src/org/partiql/cli/main.kt 0.00% <0.00%> (ø)
cli/src/org/partiql/cli/streams.kt 50.00% <ø> (+50.00%) ⬆️
cli/src/org/partiql/cli/Cli.kt 89.47% <100.00%> (+10.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36e20d1...60242ec. Read the comment docs.

Copy link
Contributor

@abhikuhikar abhikuhikar left a comment

Choose a reason for hiding this comment

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

Need to fix the behavior for binding named input_data. Also consider modifying Cli.md markdown to reflect the updated behavior.

import joptsimple.*
import org.partiql.cli.functions.*
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: replace the wildcard imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will apply this comment in the next commit.

Comment on lines 133 to 140
@Test
fun withoutInput() {
val subject = makeCli("SELECT * FROM input_data")
val actual = subject.runAndOutput()

assertEquals("\n", actual)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior is incorrect. If the --input option is not provided this query should throw an error - No such binding found for 'input_data'. Right now input_data is bound to emptyInputStream. Consider removing this binding in case the cli option --input is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will apply this comment in the next commit.

@lziq lziq merged commit 3b55f47 into main Nov 29, 2021
@lziq lziq deleted the CLI-input-arg branch November 29, 2021 19:52
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.

Add ability to use --query command-line option without external data source
3 participants