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

Set up PyDough AST module with basic abstract classes and simple type verifiers #13

Merged
merged 19 commits into from
Oct 29, 2024

Conversation

knassre-bodo
Copy link
Contributor

@knassre-bodo knassre-bodo commented Oct 24, 2024

Defines the PyDough AST module with the following:

  • Abstract base class PyDoughAST for all AST nodes
  • PyDoughASTException exception class
  • Abstract base class PyDoughExpressionAST for all AST expression nodes
  • PyDough operators module containing a type inference module
  • Within the type inference module, contains a type verifier base class and an implementation that always-accepts, and one that requires a specific # of arguments.
  • Sets up testing suite for type verifiers

assert pydough_type.json_string == type_string
assert (
repr(pydough_type) == repr_string
), "parsed type does not match expected repr() string"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency.

Comment on lines 11 to 19
@pytest.mark.parametrize(
"verifier, args",
[pytest.param(AllowAny(), [], id="allow_any-empty_args")],
)
def test_verification(verifier: TypeVerifier, args: List[PyDoughAST]):
"""
Checks that verifiers accept certain arguments without raising an exception
"""
verifier.accepts(args)
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 dumb test, but setting up the framework & verifying that the imports don't cause a build error.

@@ -0,0 +1,38 @@
"""
Copy link
Contributor Author

@knassre-bodo knassre-bodo Oct 25, 2024

Choose a reason for hiding this comment

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

This file will eventually be filled by a lot of verifiers that borrow a lot of ideas from Calcite, such as:

  • Signatures
  • Compounds
  • Allow a specific type
  • Require membership to a family
  • Require matches across arguments
  • Variadics
  • Literal matching (e.g. DATE('now', 'start of month', '-3 months'))

However, these will usually be loose requirements that represent a minimal amount of universally accepted behavior (sound-but-not-complete), such as:

  • x + y requires the arguments to be numeric
  • LOWER(...) requires the argument to be a string
  • Datetime extraction functions require a datetime argument.
  • Usually, if the type is unknown, just allow it.

The main thing is making sure that things that should be expressions vs collections are actually expressions vs collections (e.g. no calling COALESCE on tpch.Parts). That kind of check will come into play later.

@knassre-bodo knassre-bodo requested a review from njriasan October 28, 2024 13:27
@knassre-bodo knassre-bodo changed the base branch from main to kian/update_metadata_imports October 28, 2024 15:37
Base automatically changed from kian/update_metadata_imports to main October 28, 2024 15:48
from abc import ABC, abstractmethod


class PyDoughAST(ABC):
Copy link
Contributor Author

@knassre-bodo knassre-bodo Oct 28, 2024

Choose a reason for hiding this comment

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

Once I start adding implementations, will create a builder class for ease of testing.

@@ -0,0 +1,46 @@
"""
TODO: add file-level docstring.
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some simple tests for now. Will become far more elaborate once I have node builders.

Copy link
Contributor

@njriasan njriasan left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @knassre-bodo

def __eq__(self, other):
return self.equals(other)

@abstractmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is just to make an abstract method since you can't make __eq__ abstract?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More or less, yes

@knassre-bodo knassre-bodo merged commit cc22bf4 into main Oct 29, 2024
4 checks passed
@knassre-bodo knassre-bodo deleted the kian/setup_type_verifiers branch October 29, 2024 17:10
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.

2 participants