Skip to content

Dev/gfql datetimes#671

Closed
lmeyerov wants to merge 1196 commits into
masterfrom
dev/gfql-datetimes
Closed

Dev/gfql datetimes#671
lmeyerov wants to merge 1196 commits into
masterfrom
dev/gfql-datetimes

Conversation

@lmeyerov

@lmeyerov lmeyerov commented Jun 22, 2025

Copy link
Copy Markdown
Contributor

Summary

Add temporal (datetime/date/time) support to GFQL comparison predicates and is_in operator, enabling filtering by temporal values with proper timezone handling.

Changes

Features

  • Temporal predicate support: gt, lt, ge, le, eq, ne, between, and is_in now accept datetime, date, and time values
  • Timezone awareness: Full support for timezone-aware datetime comparisons
  • Wire protocol: JSON serialization format for temporal values with type, value, and optional timezone fields
  • AST classes: New DateTimeValue, DateValue, TimeValue classes in graphistry.compute.ast_temporal

Type System Improvements

  • Replace Any types with proper type annotations:
    • ComparisonInput for comparison predicates
    • IsInElementInput for is_in predicate
    • BetweenBoundInput for between predicate
  • Add type guards with TypeGuard annotations for better type narrowing
  • Fix Python 3.10 compatibility issues

Code Organization

  • Move temporal value classes from predicates/temporal_values.py to compute/ast_temporal.py
  • Rename numeric.py to comparison.py (more accurate module name)
  • Extract temporal coercion functions to models/gfql/coercions/temporal.py
  • Add missing __init__.py files for proper package structure

Testing

  • Comprehensive temporal predicate tests including timezone handling
  • Fix import errors in test files
  • All tests, lint, and type checks passing

Examples

from graphistry import n, chain
from graphistry.compute.predicates import gt, between
import pandas as pd
from datetime import date, time

# Filter by datetime
g.chain([n({"timestamp": gt(pd.Timestamp("2023-01-01"))})])

# Filter by date range
g.chain([n({"date": between(date(2023, 6, 1), date(2023, 6, 30))})])

# Filter by time of day
g.chain([n({"time": between(time(9, 0), time(17, 0))})])

Related PRs

Fixes #660

Comment thread graphistry/compute/predicates/is_in.py
Comment thread graphistry/compute/predicates/numeric.py
- Remove predicates/__init__.py (not needed, compute/__init__ imports directly)
- Replace Any types with proper type annotations using @overload
- Add TypedDict classes for wire protocol (DateTimeDict, DateDict, TimeDict)
- Rename numeric.py → comparison.py (more accurate since it handles both numeric and temporal)
- Update all imports to use new comparison module name

All tests pass, no breaking changes to public API.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@lmeyerov

Copy link
Copy Markdown
Contributor Author

Changes Made to Address PR Feedback

I've addressed the critical code quality issues:

✅ Completed

  1. Removed __init__.py - Deleted the unnecessary predicates module init file
  2. Fixed Any types - Replaced with proper type annotations using @overload and specific Union types
  3. Renamed numeric.pycomparison.py - More accurate name since it handles both numeric and temporal comparisons
  4. Improved dict typing - Created TypedDict classes (DateTimeDict, DateDict, TimeDict) instead of generic dict

🔄 Still TODO

  • Documentation updates (notebook and docs improvements)
  • Consider factoring out normalization logic further
  • CHANGELOG.md issue (appears to be from earlier commits)

All tests pass locally and in Docker. CI is running now.

The changes maintain full backward compatibility - no breaking changes to the public API.

lmeyerov and others added 5 commits June 22, 2025 12:56
The merge brought in a truncated CHANGELOG. This restores the full
history from master and adds our temporal predicate changes to the
Dev section.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…rmalize_comparison_value

- Remove class_name parameter that was only used in error messages
- Update all call sites in comparison.py
- Fix mypy errors by properly handling timezone types and reordering overloads
- Fix test import after numeric.py was renamed to comparison.py

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Fixed notebook cell type (markdown vs code) issue
- Added mini TOC and What's Next section to temporal_predicates.ipynb
- Enhanced datetime_filtering.md with imports, standards info, and duration notes
- Reorganized wire_protocol_examples.md with clearer structure
- Fixed cross-reference links in documentation

Co-Authored-By: Claude <noreply@anthropic.com>
- Replace Any types with specific ComparisonInput, IsInElementInput types
- Remove Dict[str, Any] from IsInElementInput (too permissive)
- Import and use proper types in predicates
- Add Union imports to type guard module
- Tests still pass, mypy has some remaining issues with type narrowing

Co-Authored-By: Claude <noreply@anthropic.com>
- Remove temporal_predicates.ipynb demo
- Remove datetime_filtering.md documentation
- Remove wire_protocol_examples.md documentation
- These will be added in a separate stacked PR

Co-Authored-By: Claude <noreply@anthropic.com>
lmeyerov and others added 3 commits June 22, 2025 15:57
- Reset all docs/ to master state
- These API doc changes will be in the stacked docs PR
- Keeps main PR focused on code changes only

Co-Authored-By: Claude <noreply@anthropic.com>
- Remove all .rst file changes from main PR
- These belong in the stacked docs PR
- Remove documentation-related items
- Add details about temporal value classes and wire protocol
- Add details about type annotation improvements
- Add fixed section for type checking and import fixes


# For backward compatibility - keep but deprecated
class NumericASTPredicate(ComparisonPredicate):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

shouldn't there be an annotation or something in that case?

return val
elif is_any_temporal(val):
return to_ast(val)
elif is_string(val):

@lmeyerov lmeyerov Jun 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

don't we need strings for something like string between calls? are there any calls that need strings here?

else:
raise TypeError(f"Unsupported type for {self.__class__.__name__}: {type(val)}")

def _temporal_comparison(self, s: SeriesT, temporal_val: TemporalValue, op: str) -> SeriesT:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

'op' should be a literal type..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

also, instead of a big dispatch here, does it make more sense to inline these alongside their numeric counterparts below?

Comment thread graphistry/compute/predicates/is_in.py Outdated
from ..ast_temporal import TemporalValue, DateTimeValue, DateValue, TimeValue
from ...models.gfql.coercions.temporal import to_native
from ...models.gfql.types.guards import is_basic_scalar, is_any_temporal
from ...models.gfql.types.predicates import IsInElementInput

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

these imports are hard to maintain, do as graphistry.models. etc instead of relative

Comment thread graphistry/compute/predicates/is_in.py Outdated
normalized.append(self._normalize_value(val))
return normalized

def _normalize_value(self, val: IsInElementInput) -> Any:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is confusing, should normalize to some named type made out of known variants

Comment thread graphistry/compute/predicates/is_in.py Outdated
def __init__(self, options: List[IsInElementInput]) -> None:
self.options = self._normalize_options(options)

def _normalize_options(self, options: List[IsInElementInput]) -> List[Any]:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no Any


def __call__(self, s: SeriesT) -> SeriesT:
# Check if we have any temporal values in options
has_temporal = any(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

should we check all temporal vs all numeric, and erorr if mixed?

- Remove unnecessary cast() calls that were causing mypy errors
- Simplify temporal value type conversions in comparison predicates
- Let mypy infer types from to_ast() return annotation
Comment thread graphistry/models/gfql/types/predicates.py
Comment thread graphistry/models/gfql/types/predicates.py
Comment thread graphistry/utils/lazy_import.py Outdated
import torch.nn.functional as F
from graphistry.networks import HeteroEmbed
from tqdm import trange
from tqdm import trange # type: ignore[import]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

remove

Major improvements:
- Replace all Any types with proper type annotations
- Add validation to prevent mixing temporal and numeric in is_in
- Use Literal types for comparison operators
- Move to absolute imports instead of relative
- Factor out shared normalization logic
- Add proper TypedDict usage for wire formats
- Fix lint issues (line length, unused imports, trailing whitespace)

Breaking changes:
- is_in now raises ValueError when mixing temporal and numeric values
- Comparison operators now use proper typed parameters

Addresses PR comments on type safety and code organization.
@lmeyerov lmeyerov force-pushed the dev/gfql-datetimes branch from e1d57c3 to 2b2fedb Compare June 23, 2025 00:28
- Remove empty __init__.py files that were added to PR
- Fix NormalizedIsInElement type to include all temporal types
- Fix Between class to pass original inputs to sub-predicates
- Fix type annotation in is_in validation method

Resolves 8 mypy type errors. All tests pass.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@lmeyerov lmeyerov force-pushed the dev/gfql-datetimes branch from 09cf8b5 to 5dbf8ce Compare June 23, 2025 01:11
@lmeyerov lmeyerov closed this Jun 23, 2025
@lmeyerov lmeyerov deleted the dev/gfql-datetimes branch June 23, 2025 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] embed_utils.py breaks StreamHandler terminator for *all* log handlers

8 participants