Address review comments

kevjumba · kevjumba · commit 13fa653836da · 2022-06-17T09:58:06.000-07:00
Signed-off-by: Kevin Zhang &lt;kzhang@tecton.ai&gt;
diff --git a/sdk/python/feast/feature_store.py b/sdk/python/feast/feature_store.py
@@ -1371,7 +1371,7 @@ def write_to_online_store(
         provider.ingest_df(feature_view, entities, df)
 
     @log_exceptions_and_usage
-    def write_to_offline_store(
+    def _write_to_offline_store(
         self,
         feature_view_name: str,
         df: pd.DataFrame,
diff --git a/sdk/python/feast/infra/offline_stores/file.py b/sdk/python/feast/infra/offline_stores/file.py
@@ -407,23 +407,23 @@ def write_logged_features(
     @staticmethod
     def offline_write_batch(
         config: RepoConfig,
-        table: FeatureView,
+        feature_view: FeatureView,
         data: pyarrow.Table,
         progress: Optional[Callable[[int], Any]],
     ):
-        if not table.batch_source:
+        if not feature_view.batch_source:
             raise ValueError(
                 "feature view does not have a batch source to persist offline data"
             )
         if not isinstance(config.offline_store, FileOfflineStoreConfig):
             raise ValueError(
                 f"offline store config is of type {type(config.offline_store)} when file type required"
             )
-        if not isinstance(table.batch_source, FileSource):
+        if not isinstance(feature_view.batch_source, FileSource):
             raise ValueError(
-                f"feature view batch source is {type(table.batch_source)} not file source"
+                f"feature view batch source is {type(feature_view.batch_source)} not file source"
             )
-        file_options = table.batch_source.file_options
+        file_options = feature_view.batch_source.file_options
         filesystem, path = FileSource.create_filesystem_and_path(
             file_options.uri, file_options.s3_endpoint_override
         )
diff --git a/sdk/python/feast/infra/offline_stores/offline_store.py b/sdk/python/feast/infra/offline_stores/offline_store.py
@@ -274,7 +274,7 @@ def write_logged_features(
     @staticmethod
     def offline_write_batch(
         config: RepoConfig,
-        table: FeatureView,
+        feature_view: FeatureView,
         data: pyarrow.Table,
         progress: Optional[Callable[[int], Any]],
     ):
diff --git a/sdk/python/feast/infra/passthrough_provider.py b/sdk/python/feast/infra/passthrough_provider.py
@@ -109,9 +109,6 @@ def offline_write_batch(
     ) -> None:
         set_usage_attribute("provider", self.__class__.__name__)
 
-        if "created" not in data.column_names:
-            raise ValueError("input dataframe must have a created timestamp column")
-
         if self.offline_store:
             self.offline_store.offline_write_batch(config, table, data, progress)
 
diff --git a/sdk/python/tests/integration/offline_store/test_offline_push.py b/sdk/python/tests/integration/offline_store/test_offline_push.py
diff --git a/sdk/python/tests/integration/offline_store/test_offline_write.py b/sdk/python/tests/integration/offline_store/test_offline_write.py
@@ -53,7 +53,7 @@ def test_writing_incorrect_order_fails(environment, universal_data_sources):
         },
     )
     with pytest.raises(ValueError):
-        store.write_to_offline_store(
+        store._write_to_offline_store(
             driver_stats.name, expected_df, allow_registry_cache=False
         )
 
@@ -101,7 +101,7 @@ def test_writing_incorrect_schema_fails(environment, universal_data_sources):
         },
     )
     with pytest.raises(ValueError):
-        store.write_to_offline_store(
+        store._write_to_offline_store(
             driver_stats.name, expected_df, allow_registry_cache=False
         )
 
@@ -153,7 +153,7 @@ def test_writing_consecutively_to_offline_store(environment, universal_data_sour
             "created": [ts, ts],
         },
     )
-    store.write_to_offline_store(
+    store._write_to_offline_store(
         driver_stats.name, first_df, allow_registry_cache=False
     )
 
@@ -184,7 +184,7 @@ def test_writing_consecutively_to_offline_store(environment, universal_data_sour
         },
     )
 
-    store.write_to_offline_store(
+    store._write_to_offline_store(
         driver_stats.name, second_df, allow_registry_cache=False
     )
 

Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ def test_writing_incorrect_order_fails(environment, universal_data_sources):`
`53`	`53`	`},`
`54`	`54`	`)`
`55`	`55`	`with pytest.raises(ValueError):`
`56`		`- store.write_to_offline_store(`
	`56`	`+ store._write_to_offline_store(`
`57`	`57`	`driver_stats.name, expected_df, allow_registry_cache=False`
`58`	`58`	`)`
`59`	`59`
`@@ -101,7 +101,7 @@ def test_writing_incorrect_schema_fails(environment, universal_data_sources):`
`101`	`101`	`},`
`102`	`102`	`)`
`103`	`103`	`with pytest.raises(ValueError):`
`104`		`- store.write_to_offline_store(`
	`104`	`+ store._write_to_offline_store(`
`105`	`105`	`driver_stats.name, expected_df, allow_registry_cache=False`
`106`	`106`	`)`
`107`	`107`
`@@ -153,7 +153,7 @@ def test_writing_consecutively_to_offline_store(environment, universal_data_sour`
`153`	`153`	`"created": [ts, ts],`
`154`	`154`	`},`
`155`	`155`	`)`
`156`		`- store.write_to_offline_store(`
	`156`	`+ store._write_to_offline_store(`
`157`	`157`	`driver_stats.name, first_df, allow_registry_cache=False`
`158`	`158`	`)`
`159`	`159`
`@@ -184,7 +184,7 @@ def test_writing_consecutively_to_offline_store(environment, universal_data_sour`
`184`	`184`	`},`
`185`	`185`	`)`
`186`	`186`
`187`		`- store.write_to_offline_store(`
	`187`	`+ store._write_to_offline_store(`
`188`	`188`	`driver_stats.name, second_df, allow_registry_cache=False`
`189`	`189`	`)`
`190`	`190`