feat: add antlr grammar for testcase file #2

scgkiran · 2024-09-02T15:57:37Z

Coverage tool output

/extensions/functions_arithmetic_decimal.yaml 		add(dec, dec): 0
/extensions/functions_arithmetic_decimal.yaml 		subtract(dec, dec): 0
/extensions/functions_arithmetic_decimal.yaml 		multiply(dec, dec): 0
/extensions/functions_arithmetic_decimal.yaml 		divide(dec, dec): 0
/extensions/functions_arithmetic_decimal.yaml 		modulus(dec, dec): 0
/extensions/functions_arithmetic_decimal.yaml 		power(dec, dec): 9
. . .
. . .
/extensions/functions_aggregate_decimal_output.yaml 		approx_count_distinct(any): 0
/extensions/functions_aggregate_decimal_output.yaml 		count(any): 0
/extensions/functions_aggregate_decimal_output.yaml 		count(): 0

Total test count: 37, 12/510 function variants are covered

github-actions · 2024-09-02T15:57:58Z

ACTION NEEDED

Substrait follows the Conventional Commits
specification for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

grammar/SubstraitType.g4

anshuldata · 2024-09-04T04:55:59Z

grammar/SubstraitType.g4

+  | PrecisionTimestamp isnull=QMark? Lt precision=numericParameter Gt #precisionTimestamp
+  | PrecisionTimestampTZ isnull=QMark? Lt precision=numericParameter Gt #precisionTimestampTZ
+  | Struct isnull=QMark? Lt expr (Comma expr)* Gt #struct
+  | NStruct isnull=QMark? Lt Identifier expr (Comma Identifier expr)* Gt #nStruct


I don't see Nstruct in type.proto (here). Where it is coming from ?

https://substrait.io/types/named_structs/

anshuldata · 2024-09-04T04:57:38Z

grammar/SubstraitType.g4

+  | Decimal isnull=QMark? Lt precision=numericParameter Comma scale=numericParameter Gt #decimal
+  | PrecisionTimestamp isnull=QMark? Lt precision=numericParameter Gt #precisionTimestamp
+  | PrecisionTimestampTZ isnull=QMark? Lt precision=numericParameter Gt #precisionTimestampTZ
+  | Struct isnull=QMark? Lt expr (Comma expr)* Gt #struct


Shouldn't struct take type. In proto file it takes type (here)

Suggested change

| Struct isnull=QMark? Lt expr (Comma expr)* Gt #struct

| Struct isnull=QMark? Lt type (Comma type)* Gt #struct

expr is type expression

anshuldata · 2024-09-04T04:58:16Z

grammar/SubstraitType.g4

+  | PrecisionTimestampTZ isnull=QMark? Lt precision=numericParameter Gt #precisionTimestampTZ
+  | Struct isnull=QMark? Lt expr (Comma expr)* Gt #struct
+  | NStruct isnull=QMark? Lt Identifier expr (Comma Identifier expr)* Gt #nStruct
+  | List isnull=QMark? Lt expr Gt #list


Same comment as struct. Proto file mentions type (here)

Suggested change

| List isnull=QMark? Lt expr Gt #list

| List isnull=QMark? Lt type Gt #list

anshuldata · 2024-09-04T04:58:49Z

grammar/SubstraitType.g4

+  | Struct isnull=QMark? Lt expr (Comma expr)* Gt #struct
+  | NStruct isnull=QMark? Lt Identifier expr (Comma Identifier expr)* Gt #nStruct
+  | List isnull=QMark? Lt expr Gt #list
+  | Map isnull=QMark? Lt key=expr Comma value=expr Gt #map


Same comment as struct. Proto file mentions type (here)

Suggested change

| Map isnull=QMark? Lt key=expr Comma value=expr Gt #map

| Map isnull=QMark? Lt key=type Comma value=type Gt #map

anshuldata

I haven't reviewed it fully. I am still halfway

jacques-n · 2024-09-05T01:37:47Z

tests/coverage/antlr_parser/TestFileLexer.interp

@@ -0,0 +1,330 @@
+token literal names:


I'm not sure how others feel but seeing file names that are Test* makes me think they are test files as opposed to files that are responsible parsing a test file format. Maybe some other naming pattern could make that clearer.

how about FuncTestSpec as prefix?
This file will become FuncTestSpecLexer

jacques-n · 2024-09-05T01:39:07Z

tests/coverage/extensions.py

+    "i8": "i8",
+    "i16": "i16",
+    "i32": "i32",
+    "i64": "i64",


is there a way we can push these patterns into the parser for use across tools? I think we need them to go from in grammar literal to types. Let's just do that with the grammar.

I made a change to use rule names, and added a test to assert to ensure names do not change. Grammar is not using simple literals for type names to allow case-insensitive names.

jacques-n · 2024-09-05T01:39:56Z

tests/coverage/extensions.py

+short_type_to_type = {st: lt for lt, st in type_to_short_type.items()}
+
+
+class Extension:


it isn't clear why we need this stuff. seems like the grammar could handle and move us straight to appropriate objects in many (most?) cases.

This is needed to build function registry from extensions which will be used to compute test coverage for each function variant.

tests/coverage/test_coverage.py

jacques-n · 2024-09-05T01:43:02Z

tests/test_extensions.py

@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: Apache-2.0
+def test_read_substrait_extensions():


should add to github precommit tests.

tests/coverage/coverage.py

scgkiran added 7 commits September 2, 2024 21:23

Antlr grammar for SubstraitTypes and test file format

bb59315

Some fixes still not working

ef92bca

Parser changes to parse a test file and populate TestCase in an object

feff8b3

Changes to load extension files

1aa164b

Initial changes for coverage tool

547b76b

coverage tool fixes

58249db

Add auto-generated parser files

adcce01

scgkiran mentioned this pull request Sep 2, 2024

feat: add ANTLR grammar for testcase file #1

Closed

2 tasks

Fix python style-check

1f1b635

scgkiran changed the title ~~WIP: feat: add antlr grammar for testcase file~~ feat: add antlr grammar for testcase file Sep 3, 2024

scgkiran added 4 commits September 3, 2024 19:37

Fix python style-check 2

6478c34

Fix python style-check 3

d783159

Fix python style-check 4

283bfd2

Get rid of unused file

1830e63

scgkiran marked this pull request as ready for review September 3, 2024 15:54

anshuldata reviewed Sep 4, 2024

View reviewed changes

review comments

00feec6

jacques-n reviewed Sep 5, 2024

View reviewed changes

scgkiran added 6 commits September 12, 2024 20:02

review comments 1

8305ee3

code style

39ff792

fix to use specific imports

8ec50b5

licensing and lint errors

924f441

fix few more lint errors

5a36340

Add check-substrait-extensions to pre-commit

0f672bc

scgkiran force-pushed the SD-7879 branch from b8efe5c to 0f672bc Compare September 12, 2024 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add antlr grammar for testcase file #2

feat: add antlr grammar for testcase file #2

scgkiran commented Sep 2, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024

anshuldata Sep 4, 2024

scgkiran Sep 13, 2024

anshuldata Sep 4, 2024

scgkiran Sep 13, 2024

anshuldata Sep 4, 2024

anshuldata Sep 4, 2024

anshuldata left a comment

jacques-n Sep 5, 2024

scgkiran Sep 11, 2024 •

edited

Loading

jacques-n Sep 5, 2024

scgkiran Sep 12, 2024

jacques-n Sep 5, 2024

scgkiran Sep 12, 2024

jacques-n Sep 5, 2024

scgkiran Sep 12, 2024

	\| Struct isnull=QMark? Lt expr (Comma expr)* Gt #struct
	\| Struct isnull=QMark? Lt type (Comma type)* Gt #struct

	\| List isnull=QMark? Lt expr Gt #list
	\| List isnull=QMark? Lt type Gt #list

	\| Map isnull=QMark? Lt key=expr Comma value=expr Gt #map
	\| Map isnull=QMark? Lt key=type Comma value=type Gt #map

		short_type_to_type = {st: lt for lt, st in type_to_short_type.items()}


		class Extension:

		@@ -0,0 +1,12 @@
		# SPDX-License-Identifier: Apache-2.0
		def test_read_substrait_extensions():

feat: add antlr grammar for testcase file #2

Are you sure you want to change the base?

feat: add antlr grammar for testcase file #2

Conversation

scgkiran commented Sep 2, 2024 • edited Loading

github-actions bot commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshuldata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scgkiran Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scgkiran commented Sep 2, 2024 •

edited

Loading

scgkiran Sep 11, 2024 •

edited

Loading