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.

Sign up for GitHub

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

feat: add test file format #680

Closed
wants to merge 5 commits into from
Closed

feat: add test file format #680

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
39de59e
Proposal for test file format
scgkiran Aug 14, 2024
d4d69e1
rename test files
scgkiran Aug 14, 2024
8676a4c
use shortType name
scgkiran Aug 15, 2024
759a755
Address review comments.
scgkiran Aug 16, 2024
3a541c0
Address review comments
scgkiran Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 157 additions & 0 deletions tests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
# Substrait Test Format

This document describes the format for Substrait scalar test files.
A test file consists of the following elements:

1. Version declaration
2. Optional include statements
3. One or more test groups, each containing one or more test cases

## Syntax

### Version Declaration
The version declaration must be the first line of the file. It specifies the version of the test file format. The version declaration must be in the following format:
```
### SUBSTRAIT_SCALAR_TEST: V1
```

### Include Statements
Include statements should have at least one include statement. The include statement specifies the path to substrait extension functions. The include statement must be in the following format:
```
### SUBSTRAIT_INCLUDE: /extensions/functions_aggregate_approx.yaml
scgkiran marked this conversation as resolved.
Show resolved Hide resolved
```

### Test Groups
scgkiran marked this conversation as resolved.
Show resolved Hide resolved
A test group is a collection of test cases that are logically related. Test groups are purely for categorization purposes and do not affect the execution or meaning of tests.
- **description**: A string describing the test group or case. The description must start with a `#` character.
```code
# Common Maths
```
### Test Cases
A test case consists of the following elements:

- **function**: The name of the function being tested. The function name must be an identifier alphanumeric string.
- **arguments**: Comma-separated list of arguments to the function. The arguments must be literals.
scgkiran marked this conversation as resolved.
Show resolved Hide resolved
- **options**: Optional comma-separated list of options in `key:value` format. The options describe the behavior of the function. The test should be run only on dialects that support the options. If options are not specified, the test should be run for all permutations of the options.
scgkiran marked this conversation as resolved.
Show resolved Hide resolved
- **result**: The expected result of the function. Either `SUBSTRAIT_ERROR` or a literal value.
- **literal**: In the format `<literal_value>::<datatype>`
- **description**: A string describing the test case

```code
add(126::i8, 1::i8) = 127::i8 # addition of two numbers
```

### Spec
Copy link
Member

Choose a reason for hiding this comment

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

If we define the specification here and in the Antlr grammar file will we end up changing both places all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, hopefully only when new types are introduced.


```
doc := <version>
(<include>)+
((<test_group>)?(<test_case>)+\n)+
version := ### SUBSTRAIT_SCALAR_TEST: <test_library_version>
include := ### SUBSTRAIT_INCLUDE: <uri>
test_group := # <description>
test_case := <function>(<arguments>) ([<options>])? = <result> (#<description>)?
description := string
function := string
Comment on lines +54 to +55
Copy link
Member

Choose a reason for hiding this comment

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

Is this the token string or is string defined somewhere? Can function have a space in it or special characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[A-Za-z0-9] [A-Za-z0-9_]*
I will redefine the grammar in antlr

Copy link
Member

Choose a reason for hiding this comment

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

I suggest using the definition of string already implemented by the textplan Antlr grammar.

arguments := <argument>, <argument>, ... <argument>
argument := <literal>
literal := <literal_value>::<datatype>
result := <substrait_error> | <literal>
options := <optLiteral>, <optLiteral>, ... <optLiteral>
Copy link
Member

Choose a reason for hiding this comment

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

Options are values passed to the functions but not Substrait literals. Probably best to avoid the confusion here.

optLiteral := <option_name>:<option_value>
scgkiran marked this conversation as resolved.
Show resolved Hide resolved
lietral_value := string | integer | decimal | float | boolean | date | time | timestamp | timestamp_tz | interval year | interval days | null
Copy link
Member

Choose a reason for hiding this comment

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

Literal

datatype := <basic_type> | <parametrized_type> | <complex_type>
basic_type := bool | i8 | i16 | i32 | i64 | f32 | f64 | str | date | time | ts | tstz | iyear | iday | <parametrized_type>
parametrized_type := dec<int,int> | fchar<int> | vchar<int> | vbin<int>
complex_type := <struct> | <list> | <map>
substrait_error := <!ERROR> | <!UNDEFINED>
```

**TODO:** use ANTLR to describe the grammar and generate parser
### Literals

`<literal_value>` described in this section.

#### String
- **string**, **fixedchar**, **varchar**: A sequence of characters enclosed in single quotes. Example: 'Hello, world!'
Copy link
Member

Choose a reason for hiding this comment

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

How are escapes handled?


#### Integer
Integers are represented as sequences of digits. Negative numbers are preceded by a minus sign.
- **i8**: 8-bit integer, range: -128 to 127
- **i16**: 16-bit integer, range: -32768 to 32767
- **i32**: 32-bit integer, range: -2147483648 to 2147483647
- **i64**: 64-bit integer, range: -9223372036854775808 to 9223372036854775807

#### Fixed Point decimals
- **decimal**: Fixed-point decimal number. Maximum 38 digits total, with up to 37 digits after the decimal point.
Example: 123.456

#### Floating Point numbers
- **float**: General floating-point number, can be represented as:
* Standard decimal notation: 123.456
* Scientific notation: 1.23e4
* Special values: `+inf` (Positive Infinity), `-inf` (Negative Infinity), `+0` (Positive Zero), `-0` (Negative Zero), `nan` (Not a Number), `snan` (Signaling NaN)
- **float32**: Single-precision float, approximately 6 significant digits, range: ~1.2e-38 to ~3.4e38
- **float64**: Double-precision float, approximately 15 significant digits, range: ~2.3e-308 to ~1.7e308
Comment on lines +94 to +95
Copy link
Member

Choose a reason for hiding this comment

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

Should a "test runner" only validate the answer within 6 digits? In other words, if one engine returns (float32) divide(1.0, 3.0) as 0.3333333432674407958984375 and another engine returns divide(1.0, 3.0) as 0.3333330452442169189453125 should the test runner consider both engines correct?


#### Boolean
- Valid values: true, false

#### Date and Time
All date and time literals use ISO 8601 format:

- **date**: `YYYY-MM-DD`, example: `2021-01-01`
- **time**: `HH:MM:SS[.fraction]`, example: `12:00:00.000`
- **timestamp**: `YYYY-MM-DD HH:MM:SS[.fraction]`, example: `2021-01-01 12:00:00`
- **timestamp_tz**: `YYYY-MM-DD HH:MM:SS[.fraction]±HH:MM`, example: `2021-01-01 12:00:00+05:30`
- **interval year**: `'P[n]Y[n]M'`, example: `'P2Y3M'` (2 years, 3 months)
Copy link
Member

Choose a reason for hiding this comment

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

What does P stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Period

- **interval days**: `'P[n]DT[n]H[n]M[n]S[n]F'`, Valid values for F are 1-9, representing fraction of a second. example: `'P2DT3H2M9S'` (2 days, 3 hours, 2 minutes, 9 seconds)
Copy link
Member

Choose a reason for hiding this comment

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

While concise this is a very difficult to read and write. For instance remembering that one of the elements has two letters -- and which one -- is difficult. Take a look at the text format for a more readable form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://en.wikipedia.org/wiki/ISO_8601#Durations

It starts with P, T is the time designator that precedes all Time elements. All elements have single letter to denote it.

ex2: 'P1DT2H3M4S99::iday<3>' (1 day, 2 hours, 3 minutes, 4 seconds, 99 milliseconds)`

#### Other complex types
**TODO** Add support for complex types like arrays, structs, maps etc.
Copy link
Member

Choose a reason for hiding this comment

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

These are already defined in the text format. You do need a way of specifying these that is amenable to recursive parsing. The Substrait C++ type format took an approach that was more error prone.


### Data Types

Use short names listed in https://substrait.io/extensions/#function-signature-compound-names

- **bool**: Boolean
- **i8**: 8-bit signed integer
- **i16**: 16-bit signed integer
- **i32**: 32-bit signed integer
- **i64**: 64-bit signed integer
- **f32**: 32-bit floating point number
- **f64**: 64-bit floating point number
- **dec**: Fixed-point `decimal<P,S>`
Copy link
Member

Choose a reason for hiding this comment

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

Do short forms still need to be parameterized? Can we refer to the list elsewhere? Can long names be interchangeably used?

Copy link
Contributor Author

@scgkiran scgkiran Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes parametrization is needed. I prefer to not to use long names in the tests.

- **str**: Variable-length string
- **fchar**: Fixed-length string `fixedchar<N>`
- **vchar**: Variable-length string `varchar<N>`
- **vbin**: Fixed-length binary `fixedbinary<N>`
- **date**: Date
- **time**: Time
- **ts**: Timestamp
- **tstz**: Timestamp with timezone
scgkiran marked this conversation as resolved.
Show resolved Hide resolved
- **iyear**: Interval year
- **iday**: Interval days


### Options

**TODO** Add option names and values to the grammar. Option names and values can be found in extension files.

### Errors

If return type is `<!ERROR>` then the test case should fail with an error. If the return type is `<!UNDEFINED>` then the test case should not fail result could be anything.
### Example of a test file

```code
### SUBSTRAIT_SCALAR_TEST:V1
### SUBSTRAIT_INCLUDE: /extensions/functions_arithmetic.yaml

# Common Maths
add(126::i8, 1::i8) = 127::i8

# Arithmetic Overflow Tests
add(127::i8, 1::i8) [overflow:ERROR] = ERROR #check overflow
Copy link
Member

Choose a reason for hiding this comment

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

Can comments sometimes be recognized as group names? Do we care?

Copy link
Contributor Author

@scgkiran scgkiran Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes. Empty groups can be ignored by the testing framework.

```
The above test file has two test groups "Common Maths" and "Arithmetic Overflow Tests". Each has one test case. The test case in the second group has a name whereas case in the first one does not.
31 changes: 31 additions & 0 deletions tests/cases/arithmetic/add.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
### SUBSTRAIT_SCALAR_TEST: 1.0
### SUBSTRAIT_INCLUDE: /extensions/functions_arithmetic.yaml

# basic: Basic examples without any special cases
Copy link
Member

Choose a reason for hiding this comment

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

Is the name the whole line or only up to the semicolon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whole line

add(120::i8, 5::i8) = 125::i8
add(100::i16, 100::i16) = 200::i16
add(30000::i32, 30000::i32) = 60000::i32
add(2000000000::i64, 2000000000::i64) = 4000000000::i64

# overflow: Examples demonstrating overflow behavior
add(120::i8, 10::i8) [overflow:ERROR] = <!ERROR>
add(30000::i16, 30000::i16) [overflow:ERROR] = <!ERROR>
add(2000000000::i32, 2000000000::i32) [overflow:ERROR] = <!ERROR>
add(9223372036854775807::i64, 1::i64) [overflow:ERROR] = <!ERROR>

# overflow: Examples demonstrating overflow behavior tests: overflow with SATURATE
add(120::i8, 10::i8) [overflow:SATURATE] = 127::i8
add(-120::i8, -10::i8) [overflow:SATURATE] = -128::i8

# overflow: Examples demonstrating overflow behavior tests: overflow with SILENT
add(120::i8, 10::i8) [overflow:SILENT] = undefined
scgkiran marked this conversation as resolved.
Show resolved Hide resolved

# floating_exception: Examples demonstrating exceptional floating point cases
add(1.5e+308::fp64, 1.5e+308::fp64) = inf::fp64
add(-1.5e+308::fp64, -1.5e+308::fp64) = -inf::fp64

# rounding: Examples demonstrating floating point rounding behavior
add(4.5::fp32, 2.500001::fp32) [rounding:TIE_TO_EVEN] = 7.000001::fp32
Comment on lines +27 to +28
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really demonstrate rounding error. If we're sticking with the 6/15 digits of precision thing then it is impossible to demonstrate rounding error. Should we just remove these tests?


# types: Examples demonstrating behavior of different data types
add(4.5::fp64, 2.5000007152557373::fp64) = 7.00000071525573::fp64
21 changes: 21 additions & 0 deletions tests/cases/arithmetic_decimal/power.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
### SUBSTRAIT_SCALAR_TEST: 1.0
### SUBSTRAIT_INCLUDE: extensions/functions_arithmetic_decimal.yaml

# basic: Basic examples without any special cases
power(8::decimal, 2::decimal<38, 0>) = 64::fp64
scgkiran marked this conversation as resolved.
Show resolved Hide resolved
power(1.0::decimal, -1.0::decimal<38, 0>) = 1.0::fp64
power(2.0::decimal<38, 0>, -2.0::decimal<38, 0>) = 0.25::fp64
power(13::decimal<38, 0>, 10::decimal<38, 0>) = 137858491849::fp64

# result_more_than_input_precison: Examples demonstrating result with more precision than input
power(16::decimal<2, 0>, 4::decimal<38, 0>) = 65536::fp64
Copy link
Member

@EpsilonPrime EpsilonPrime Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps we could reuse the text format's definition of literals?

https://github.com/substrait-io/substrait-cpp/blob/main/src/substrait/textplan/README.md

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to stick with :: format generally. It is a cast format consistent with many systems the people looking at these tests will be familiar with and I believe much more intuitive understood than underscore.

I'm similarly inclined to try to find more standard ways to represent other things as opposed to what the text format has done. There are several examples of alternative ways to implement things to what is commonly done in SQL (which has the most consistent way to approach these things across systems). Examples beyond the use of cast syntax versus underscores include. (1) String literals are double quoted and backticked while sql string literals are single quoted and (2) brackets being overloaded to mean any of intervals, lists, maps and structs.

Copy link
Member

Choose a reason for hiding this comment

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

The :: format is a type subclass operator. As such it requires a mental context switch every time I look at it. The underscore was chosen in the text format to create a visual separation between the literal value and its type thus aiding in readability (and it's also requires less keystrokes).

Braces are used for literal values and angle brackets are used for type composition and parameterization. This helps to reinforce the distinction between value and type. I have yet to be able to determine what the value of any interval literal is in the proposed format. However the text format's use of labels makes it ultra-clear what each part of the interval is.

The text format's string implementation does draw from other sources than SQL but that's mostly because the SQL version has its own warts which make it harder to read. SQL's implicit concatenation is the main SQL string feature that is difficult for humans to parse (it requires the counting of active single quotes).

The text format was reviewed in the community meeting twice (February 1st, 2023 and April 12th, 2023) so presumably there is some community agreement about its design validity.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that the text format defined in substrait-cpp isn't valid. It's definitely valid.

I think we come from different perspectives. I'm coming from the perspective that we should try to leverage existing patterns as much as possible. My expectation is that the vast majority of systems that will be tested are SQL systems so leveraging SQL patterns is desirable as much as reasonably possible and comes with the added benefit of being obvious in meaning to a large group of people reviewing the test case. If we look at the set of BFT dialects, for example, we have the following:

Dialect API Single Quotes for Literals Supports :: for Cast
cudf cudf api no no
datafusion SQL yes no
duckdb SQL yes yes
postgres SQL yes yes
snowflake SQL yes yes
sqllite SQL yes no
velox velox api no no

So a decent portion are SQL based. Single-quoted literals are the most common way to represent strings in those systems. 3/7 have support for the double colon as meaning cast. Given that, I prefer that over something invented.


# floating_exception: Examples demonstrating exceptional floating point cases
power(1.5e+10::decimal<38, 0>, 1.5e+20::decimal<38, 0>) = inf::fp64
Copy link
Member

Choose a reason for hiding this comment

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

Should this be the short form names as noted above or are both legal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it should be short form. Fixed it.

power(-16::decimal<4, 0>, 1001::decimal<4, 0>) = -inf::fp64

# complex_number: Examples demonstrating complex number output
power(-1::decimal, 0.5::decimal<38,1>) [complex_number_result:NAN] = nan

# complex_number: Examples demonstrating complex number output tests: complex_number_result with ERROR
power(-1::decimal, 0.5::decimal<38,1>) [complex_number_result:ERROR] = <!ERROR>
25 changes: 25 additions & 0 deletions tests/cases/datetime/lt_datetime.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
### SUBSTRAIT_SCALAR_TEST: 1.0
### SUBSTRAIT_INCLUDE: /extensions/functions_datetime.yaml

# timestamps: examples using the timestamp type
lt(2016-12-31 13:30:15::ts, 2017-12-31 13:30:15::ts) = true::bool
lt(2018-12-31 13:30:15::ts, 2017-12-31 13:30:15::ts) = false::bool

# timestamp_tz: examples using the timestamp_tz type
lt(1999-01-08 01:05:05-08:00::tstz, 1999-01-08 04:05:06-05:00::tstz) = true::bool
lt(1999-01-08 01:05:06-08:00::tstz, 1999-01-08 04:05:06-05:00::tstz) = false::bool

# date: examples using the date type
lt(2020-12-30::date, 2020-12-31::date) = true::bool
lt(2020-12-31::date, 2020-12-30::date) = false::bool

# interval: examples using the interval type
lt('P7D'::iday, 'P6D'::iday) = false::bool
lt('P5D'::iday, '6D'::iday) = true::bool
Copy link
Member

Choose a reason for hiding this comment

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

Is the P here optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it was a typo. Fixed it.

lt('P5Y'::iyear, 'P6Y'::iyear) = true::bool
lt('P7Y'::iyear, 'P6Y'::iyear) = false::bool

# null_input: examples with null args or return
lt(null::iday, 'P5D'::iday) = null::bool
lt(null::date, 2020-12-30::date) = null::bool
lt(null::ts, 2018-12-31 13:30:15::ts) = null::bool
Loading