• generic pattern for AST libraries to report human friendly name of AstNode instances
• sqlwhat only uses most recent message for now, relies on highlighting for context. (e.g. rather than "Check the 1st Select statement. Check the WHERE clause. Did you _____?", Use highlighting + "Did you ____?".
• sqlwhat uses human friendly names when can't find node or field (e.g. "Did you define a {ordinal} {node_name}" -> "Did you define a 2nd SELECT statement?")

In general, producing a statement like this is very hard. It's unclear how this should be done. For reference, a WHERE clause might look like this...

(Note that we essentially run into the same problem here as for test_operator in pythonwhat.)

Currently, we use travis to build sqlwhat (by generating the antlr python targets). However, if we move the parser out, then we can move all the building to the parser, which will likely be updated less frequently than sqlwhat itself. This will also help for creating a small app to visualize the AST, since it and sqlwhat can just depend on the parser.

Many tests have the form

def test_test_has_columns_fail():
    state = prepare_state({'a': [1,2,3]}, {})
    with pytest.raises(TF): cr.test_has_columns(state)

For some of them, it would be useful to optionally view their feedback message. Could do this by doing something like

@pytest.mark.feedback
def test_something():
    ....

Then they can be viewed by running from command-line...

pytest -s -m "feedback"    # plus some meta-parameter telling tests to print feedback

should be able to pull from pythonwhat

SCT chains can store calls as actions, which can be put together in string form on fail.

Cases:

• if SCT uses test_column(), but no results

Currently, when you get the target_list of a SelectStmt, you get a literal python list. That's fine for comparing currently, since things like has_equal_ast use repr to compare AST representations. However, it could cause problems down the road.

has_equal_ast will be equivalent to using override_sql -> testing the asts are identical

Query

SELECT DISTINCT country
FROM films;

SCT

distinct = sel.check_field('pref').has_equal_ast(msg='blah')

sqlwhat-viewer

Error

Question

Has the field for DISTINCT changed to something else?

cc @ncarchedi

Swipe it from pythonwhattt

Overview

Currently check_result() will pass if the columns in the student submission are some superset of the columns in the solution.

This means that

SELECT *
FROM films;

will pass, when the desired solution is:

SELECT title, release_year
FROM films;

Enhancement Request

Adding test_ncols() to check_result() so the number of columns in student submission must match the number of columns in the solution code. I would argue that this is desirable default behavior, but it's up for discussion: cc @ncarchedi

Enhancement request:

duration_days --> duration_days

• can be skipped to get next statement (e.g. check select stmt -> check select stmt)
• missing msg, if attribute is None
• can be followed by has_equal_ast

This is since a good, robust message for has_equal_ast is going to take time, and exercise data to figure out.

Similar to pythonwhat. This will allow SCTs like

*** =solution

SELECT * 
FROM film
WHERE release_year = 2006
AND film_id < 5;

*** =sct

Ex().check_node('SelectStmt') \
    .check_field('where_clause') \
    .has_equal_ast(sql = "film_id < 5", start="expression", exact = False)

Note that we'll need to work out how to show CDs the names of parser rules in sqlwhat-viewer (so they can specify the start arg).

Currently, to workaround tsql issues...

1. AST representations are converted to lowercase
2. Column names are converted to lowercase

In postgres, these are not issues because unquoted column names should be made lowercase in the AST (as the official parser does). This also means that column names tend to be lowercase, too. Using casing is possible, though, so we don't want to convert everything to lowercase for postgres.

When checking columns, there should be an option to compare the columns without accounting for the order of the rows. This can be done brutally, by simply ordering the column and seeing what happens, or ordering by a certain column, and keeping the permutation to order other columns accordingly (which is more correct I think).
As this can raise the number of steps in checking quite a bit, I think you should make this opt-in to start with.

e.g. SELECT * FROM artists LIMIT 10

PEC

CREATE TABLE simple ( id INTEGER);
INSERT INTO simple (1) (2)

Solution

SELECT * FROM simple

Submission

SELECT * FROM simple LIMIT 1

Raised by @colinricardo in https://github.com/datacamp/courses-msft-transact-sql/issues/49#issuecomment-302792248

See screencast: https://v.usetapes.com/WayrkiARsE

Basically, adding blanks can have the exercise pass the SCT.

It's possible that students will submit code that is valid postgresql, but the parser can't handle. In this case..

1. It should raise (e.g.) a ParsingError, that is caught and short-circuits chaining
2. Things like checking results should still run

for ctx = ast_node._ctx, should be able to use something like

start = ctx.start     # starting token
start.start              # it's first letter
start.stop
start.line

stop = ctx.stop      # stopping token
# etc..

(I'm not sure what happens if a token spans multiple lines, but also can't think of when that would happen)

Feedback from Graeme, Microsoft contact for the transact-sql course:

Also, I noticed that I get an error based on the column name case not matching the expected answer (e.g. if I were to use the alias CustomerName when it expects Customername) – by default , Transact-SQL is case-insensitive so this should not cause an error. Can you fix that?

I suggest we put an option in check_result() and test_column() to be forgiving for case sensitivity. Pretty urgent.

@ncarchedi commented on Wed Apr 26 2017

Summary of Issue

In sqlwhat, the success message for PlainMultipleChoiceExercises is always Great work!, overriding whatever custom message has been specified by the author.

Steps to Reproduce

Go here and submit the correct answer. The success message should be different.

Screenshots/Links

@ramnathv commented on Thu Apr 27 2017

@filipsch @machow can you take a look at sqlwhat?

If I understand the spec correctly people should be able to specify that these two options are both correct even when testing ASTs...

SELECT id FROM sometable ORDER BY id ASC

and

SELECT id as id2 FROM sometable ORDER BY id2 ASC

However, if the order by clause is incorrect in the second give feedback:

-- "incorrect ORDER BY clause"
SELECT id as id2 FROM sometable ORDER BY id2 DESC

implementation

This will require mapping identifiers from the solution onto the submission AST. An analogous case is mapping imports in pythonwhat. One solution is, like pythonwhat, keep a dictionary that maps each identifier and alias to the table it is referring to. E.g.

# where these are encountered in order
id as id2 -> sometable.id
id -> sometable.id
id2 -> sometable.id
sometable.id -> sometable.id

Note that this doesn't account for cases where an alias is given to, say, the result of COUNT or a CASE expression. For those situations it would just map to the corresponding COUNT or CASE ASTs...

# for CASE ... as id2
id2 -> CASE AST
# for COUNT(*) as count
count -> COUNT(*)

this will let us fix mis-parsings quickly, and avoid regressions.

related to #26 (since need to know where in raw text we are)

Minimal example:

CREATE TABLE simple ( id INTEGER)
INSERT INTO simple (1)

Pass both SELECT id FROM simple and SELECT id as id2 FROM simple.

Realistic Example:

For example, both should pass:

SELECT 
    EXTRACT(MONTH FROM start_time) AS month, 
    COUNT(*) 
FROM trips 
GROUP BY month;

SELECT 
    EXTRACT(MONTH FROM start_time) AS month, 
    COUNT(*) as ttl
FROM trips 
GROUP BY month;

Most of these just involve removing the keyword

• FROM list of tables: SELECT a FROM b, c
• WHERE
• GROUP BY
• UNION
• ORDER BY
• OFFSET (not in parser, see issue #45 )

(bonus: HAVING, WINDOW, FETCH, FOR)

For example:

AVG(duration) / 60 and AVG(duration / 60) should both be accepted.

Is the best way to do this using test_or() with test_student_typed()?

Am I doing something wrong?

See screenshot below:

Test is on line 1071, and works fine when fixed=True but I want to check for and|AND.

Another example is here:

Currently the three functions

• check_result
• check_statement
• check_clause

all begin with check, but only the last two change state (by fetching a new AST node). It seems like we should change check_result, whose behavior will change depending on the state (e.g. called after check_statement('select', ...)) to test or test_result.

The first comes from the backend, and is passed to test_exercise. The second is from running the SCT.

Should...

1. add test_error function, which fails if backend sent in an error.
2. ensure that if SCT completes, and there is already an error, then it fails and returns the original error message.

Note that this does not include the case where someone wants an exercise to succeed, even if there is a syntax error (we can always through in an allow_error function if it comes to it, seems unlikely).

for debugging, highlight checking

edit:
UNION is treated as compound statement, so for two select statements, a trailing ORDER BY is the outermost piece. It's probably okay for now, but the ORDER BY will incorrectly be part of the final SELECT statement. Example below.

SELECT a FROM b
UNION
SELECT x FROM y
ORDER BY c

the postgres ast is

  [{"SelectStmt"=>
     {"sortClause"=>
       [{"SortBy"=>
          {"node"=>
            {"ColumnRef"=>
              {"fields"=>[{"String"=>{"str"=>"c"}}], "location"=>47}},
           "sortby_dir"=>0,
           "sortby_nulls"=>0,
           "location"=>-1}}],
      "op"=>1,
      "larg"=>
       {"SelectStmt"=>
         {"targetList"=>
           [{"ResTarget"=>
              {"val"=>
                {"ColumnRef"=>
                  {"fields"=>[{"String"=>{"str"=>"a"}}], "location"=>7}},
               "location"=>7}}],
          "fromClause"=>
           [{"RangeVar"=>
              {"relname"=>"b",
               "inhOpt"=>2,
               "relpersistence"=>"p",
               "location"=>14}}],
          "op"=>0}},
      "rarg"=>
       {"SelectStmt"=>
         {"targetList"=>
           [{"ResTarget"=>
              {"val"=>
                {"ColumnRef"=>
                  {"fields"=>[{"String"=>{"str"=>"x"}}], "location"=>29}},
               "location"=>29}}],
          "fromClause"=>
           [{"RangeVar"=>
              {"relname"=>"y",
               "inhOpt"=>2,
               "relpersistence"=>"p",
               "location"=>36}}],
          "op"=>0}}}}],

Summary of Issue

At present we can check if a student typed something in the submission. It would be useful to be able to check that they did not type something.

Example

Currently for the TransactSQL course, the triple underscore (___ ) sometimes causes a parsing error in sqlwhat.

Additionally, for exercises using ISQL, at present we can only check the AST.

Enhancement Idea

So, it might be useful to check that there is no ___ present in the submission using test_student_did_not_type(), rather than failing when trying to parse the submission. Thus in the SCT, test_student_did_not_type() would only pass and allow the AST checking to occur if there were no blanks present.

Related to #96